jffs2: safely remove obsolete dirent from the f->dents list

We may delete a bunch of directory entries, operating such as:
getdents(), unlink(), getdents()..., until the end of the directory.
jffs2 handles f_pos on the directory merely as the position on the
f->dents list. So, the next getdents() may skip some entries
before f_pos, if we remove some entries from the list between two
getdents(), resulting in some entries of the directory cannot be deleted.

Commit 15953580e79b is introduced to resolve this bug by not
removing delete entries from the list immediately.

However, when CONFIG_JFFS2_SUMMARY is not set, it can cause the
following issues:

* 'deletion' dirents is always in the f->dents list, wasting memory
    resource. For example:
        There is a file named 'file1'. Then we rename it:
        mv file1 file2;
        mv file2 file3;
        ...
        mv file99999 file1000000

    All of file1~file1000000 dirents always in the f->dents list.

* Though, the list we're traversing is already ordered by CRC,
it could waste much CPU time when the list is very long.

To fix, we define two variables in struct jffs2_inode_info: nr_dir_opening,
obsolete_count, and two new functions: jffs2_dir_open(), jffs2_dir_release().

When open a directory, jffs2_dir_open() will increase nr_dir_opening,
which will be decreased by jffs2_dir_release(). if the value is 0,
it means nobody open the dir and nobody in getdents()/seek() semantics.
Thus, we can remove all obsolete dirent from the list.

When delete a file, jffs2_do_unlink() can remove the dirent directly if
nobody open the directory(i.e. nr_dir_opending == 0). Otherwise, it just
increase obsolete_count, denoting obsolete dirent count of the list.

Fixes: 15953580e79b ("[JFFS2] Improve getdents vs. f_pos handling on NOR flash.")
Signed-off-by: Yufen Yu <yuyufen@huawei.com>
---
 fs/jffs2/dir.c             | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/jffs2/jffs2_fs_i.h      |  6 ++++++
 fs/jffs2/super.c           |  4 ++++
 fs/jffs2/write.c           | 30 +++++++++++++++++++---------
 include/uapi/linux/jffs2.h |  3 +++
 5 files changed, 83 insertions(+), 9 deletions(-)

This looks a lot better than the first iteration; thank you for getting
it to this point. One last thing, I hope...
On Thu, 2018-03-29 at 20:00 +0800, Yufen Yu wrote:
> 
> --- a/fs/jffs2/jffs2_fs_i.h
> +++ b/fs/jffs2/jffs2_fs_i.h
> @@ -42,6 +42,12 @@ struct jffs2_inode_info {
>         /* Directory entries */
>         struct jffs2_full_dirent *dents;
>  
> +       /* Directory open refcount */
> +       atomic_t nr_dir_opening;
> +
> +       /* obsolete dirent count in the list of 'dents' */
> +       unsigned int obsolete_count;
> +
>         /* The target path if this is the inode of a symlink */
>         unsigned char *target;
>  

You've made JFFS2_INVALID_LIMIT 64, which is reasonable enough
(although it's a bit of a weird name and possibly wants to be more
specific — invalid *what*?).

So the maximum interesting value of ->obsolete_count is 64. Which means
it might as well be a uint8_t and sit in the padding after the
'usercompr' field.

It might be useful to look at putting the mutually exclusive fields in
struct jffs2_inode_info into a union, and then we don't need the
additional space of the atomic_t either; we'll never need that *and*
the fragtree at the same time... will we?

On 2018/4/28 6:22, David Woodhouse wrote:
> This looks a lot better than the first iteration; thank you for getting
> it to this point. One last thing, I hope...
> On Thu, 2018-03-29 at 20:00 +0800, Yufen Yu wrote:
>> --- a/fs/jffs2/jffs2_fs_i.h
>> +++ b/fs/jffs2/jffs2_fs_i.h
>> @@ -42,6 +42,12 @@ struct jffs2_inode_info {
>>          /* Directory entries */
>>          struct jffs2_full_dirent *dents;
>>   
>> +       /* Directory open refcount */
>> +       atomic_t nr_dir_opening;
>> +
>> +       /* obsolete dirent count in the list of 'dents' */
>> +       unsigned int obsolete_count;
>> +
>>          /* The target path if this is the inode of a symlink */
>>          unsigned char *target;
>>   
> You've made JFFS2_INVALID_LIMIT 64, which is reasonable enough
> (although it's a bit of a weird name and possibly wants to be more
> specific — invalid *what*?).
Thansk a lot for your suggestions.

Yes, it is really a bad name. How about JFFS2_OBS_DIRENT_LIMIT?  I am 
not sure.

>
> So the maximum interesting value of ->obsolete_count is 64. Which means
> it might as well be a uint8_t and sit in the padding after the
> 'usercompr' field.
>
> It might be useful to look at putting the mutually exclusive fields in
> struct jffs2_inode_info into a union, and then we don't need the
> additional space of the atomic_t either; we'll never need that *and*
> the fragtree at the same time... will we?
You are right, thanks. But, obsolete_count may be large. So, I apply to 
use uint16_t and
it also sits in the padding after the 'usercompr' field.

Thanks,
Yufen

On Fri, 2018-05-04 at 16:06 +0800, yuyufen wrote:
> 
> > You've made JFFS2_INVALID_LIMIT 64, which is reasonable enough
> > (although it's a bit of a weird name and possibly wants to be more
> > specific — invalid *what*?).
>
> Thansk a lot for your suggestions.
> 
> Yes, it is really a bad name. How about JFFS2_OBS_DIRENT_LIMIT?  I am 
> not sure.

That'll do; at least it's a hint in the right direction :)

> >
> > So the maximum interesting value of ->obsolete_count is 64. Which means
> > it might as well be a uint8_t and sit in the padding after the
> > 'usercompr' field.
> >
> > It might be useful to look at putting the mutually exclusive fields in
> > struct jffs2_inode_info into a union, and then we don't need the
> > additional space of the atomic_t either; we'll never need that *and*
> > the fragtree at the same time... will we?
>
> You are right, thanks. But, obsolete_count may be large. So, I apply to 
> use uint16_t and it also sits in the padding after the 'usercompr' 
> field.

You can always just cap it. Once it reaches 64 it never changes again,
until you actually harvest them. Without that, a uint16_t could
overflow too.

Hi, David.
On 2018/5/4 16:18, David Woodhouse wrote:
>
> On Fri, 2018-05-04 at 16:06 +0800, yuyufen wrote:
>>> You've made JFFS2_INVALID_LIMIT 64, which is reasonable enough
>>> (although it's a bit of a weird name and possibly wants to be more
>>> specific — invalid *what*?).
>> Thansk a lot for your suggestions.
>>
>> Yes, it is really a bad name. How about JFFS2_OBS_DIRENT_LIMIT?  I am
>> not sure.
> That'll do; at least it's a hint in the right direction :)
>
>>> So the maximum interesting value of ->obsolete_count is 64. Which means
>>> it might as well be a uint8_t and sit in the padding after the
>>> 'usercompr' field.
>>>
>>> It might be useful to look at putting the mutually exclusive fields in
>>> struct jffs2_inode_info into a union, and then we don't need the
>>> additional space of the atomic_t either; we'll never need that *and*
>>> the fragtree at the same time... will we?
>> You are right, thanks. But, obsolete_count may be large. So, I apply to
>> use uint16_t and it also sits in the padding after the 'usercompr'
>> field.
> You can always just cap it. Once it reaches 64 it never changes again,
> until you actually harvest them. Without that, a uint16_t could
> overflow too.

I think 'JFFS2_OBS_DIRENT_LIMIT' may cause misunderstanding. Sorry for that.
In my patch, it just means a threshold value. when ->nr_dir_opening is 
'0' and
  ->obsolete_count is bigger than the value, it can trigger removing 
operations.

If ->nr_dir_opening is not '0', obsolete_count may continuously increase 
and can
exceed 64.

+static int jffs2_dir_release(struct inode *dir_i, struct file *filp)
+{
+#ifndef CONFIG_JFFS2_SUMMARY
+       struct jffs2_inode_info *dir_f = JFFS2_INODE_INFO(dir_i);
+
+       BUG_ON(atomic_read(&dir_f->nr_dir_opening) <= 0);
+
+       mutex_lock(&dir_f->sem);
+       /* jffs2_dir_open may increase nr_dir_opening after
+        * atomic_dec_and_test() returning true.
+        * However, it cannot traverse the list until hold
+        * mutex dir_f->sem lock, so that we can go on
+{
+#ifndef CONFIG_JFFS2_SUMMARY
+       struct jffs2_inode_info *dir_f = JFFS2_INODE_INFO(dir_i);
+
+       BUG_ON(atomic_read(&dir_f->nr_dir_opening) <= 0);
+
+       mutex_lock(&dir_f->sem);
+       /* jffs2_dir_open may increase nr_dir_opening after
+        * atomic_dec_and_test() returning true.
+        * However, it cannot traverse the list until hold
+        * mutex dir_f->sem lock, so that we can go on
+        * removing.*/
+       if (atomic_dec_and_test(&dir_f->nr_dir_opening) &&
+                       dir_f->obsolete_count > JFFS2_OBS_DIRENT_LIMIT) {
+               struct jffs2_full_dirent **prev = &dir_f->dents;
+
+               /* remove all obsolete dirent from the list, which
+                * can save memory space and reduce CPU time for
+                * traverse the list */
+               while((*prev) && (dir_f->obsolete_count--)) {
+                       if ((*prev)->raw == NULL && (*prev)->ino == 0) {
+                               struct jffs2_full_dirent *this = *prev;
+                               *prev = this->next;
+                               jffs2_free_full_dirent(this);
+                       } else
+                               prev = &((*prev)->next);
+               }
+       }
+       mutex_unlock(&dir_f->sem);
+#endif
+
+       return 0;
+}

Thanks,
Yufen