Re: [PATCH] jffs2: remove fd from the f->dents list immediately.

[Joakim Tjernlund <Joakim.Tjernlund@infinera.com>]

On Fri, 2018-03-16 at 19:05 +0800, Yufen Yu wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> commit 15953580e79b ("[JFFS2] Improve getdents vs. f_pos handling on NOR flash.")
> is introduced to resolve 'rm -r', which cannot remove all files:
>     http://lists.infradead.org/pipermail/linux-mtd/2007-October/019658.html
> 
> However, it can cause the following issues:
> 
> 1. 'deletion' dirents is alway 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
> 
>         When CONFIG_JFFS2_SUMMARY is not set, file1~file1000000
>         always in the f->dents list.
> 
> 2. Since the list become longer and longer, more CPU time is used
>     to traverse it.
> 
> After reverting the commit, we test 'rm -r', which can remove all
> files, and all seems OK!

UHH, this is mine (and Davids work from 2007)!
I cannot remember  any details this long afterwards but I guess you cannot just
revert that part as it triggers some other bug, David?

 Jocke

> 
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> ---
>  fs/jffs2/write.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/jffs2/write.c b/fs/jffs2/write.c
> index cda9a361368e..1deed35beb50 100644
> --- a/fs/jffs2/write.c
> +++ b/fs/jffs2/write.c
> @@ -598,31 +598,32 @@ int jffs2_do_unlink(struct jffs2_sb_info *c, struct jffs2_inode_info *dir_f,
>                 jffs2_add_fd_to_list(c, fd, &dir_f->dents);
>                 mutex_unlock(&dir_f->sem);
>         } else {
> +               struct jffs2_full_dirent **prev = &dir_f->dents;
>                 uint32_t nhash = full_name_hash(NULL, name, namelen);
> 
> -               fd = dir_f->dents;
>                 /* We don't actually want to reserve any space, but we do
>                    want to be holding the alloc_sem when we write to flash */
>                 mutex_lock(&c->alloc_sem);
>                 mutex_lock(&dir_f->sem);
> 
> -               for (fd = dir_f->dents; fd; fd = fd->next) {
> -                       if (fd->nhash == nhash &&
> -                           !memcmp(fd->name, name, namelen) &&
> -                           !fd->name[namelen]) {
> -
> -                               jffs2_dbg(1, "Marking old dirent node (ino #%u) @%08x obsolete\n",
> -                                         fd->ino, ref_offset(fd->raw));
> -                               jffs2_mark_node_obsolete(c, fd->raw);
> -                               /* We don't want to remove it from the list immediately,
> -                                  because that screws up getdents()/seek() semantics even
> -                                  more than they're screwed already. Turn it into a
> -                                  node-less deletion dirent instead -- a placeholder */
> -                               fd->raw = NULL;
> -                               fd->ino = 0;
> -                               break;
> +               while ((*prev) && (*prev)->nhash <= nhash) {
> +                       if ((*prev)->nhash == nhash &&
> +                               !memcmp((*prev)->name, name, namelen) &&
> +                               !(*prev)->name[namelen]) {
> +
> +                                       struct jffs2_full_dirent *this = *prev;
> +
> +                                       jffs2_dbg(1, "Marking old dirent node (ino #%u) @%08x obsolete\n",
> +                                                       this->ino, ref_offset(this->raw));
> +                                       *prev = this->next;
> +                                       jffs2_mark_node_obsolete(c, this->raw);
> +                                       jffs2_free_full_dirent(this);
> +
> +                                       break;
>                         }
> +                       prev = &((*prev)->next);
>                 }
> +
>                 mutex_unlock(&dir_f->sem);
>         }
> 
> --
> 2.13.6
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/