Re: [PATCH 16/41] whiteout: jffs2 whiteout support

[Valerie Aurora <vaurora@redhat.com>]

On Thu, Oct 22, 2009 at 07:50:49AM +0900, David Woodhouse wrote:
> On Wed, 2009-10-21 at 12:19 -0700, Valerie Aurora wrote:
> > From: Felix Fietkau <nbd@openwrt.org>
> > 
> > Add support for whiteout dentries to jffs2.
> 
> As discussed, there are a few places where JFFS2 will assume that a
> dirent with fd->ino == 0 is a deletion dirent -- a kind of whiteout of
> its own, used internally because it's a log-structured file system and
> it needs to mark previously existing dirents as having been unlinked.
> 
> You're breaking that assumption. So, for example, your whiteouts are
> going to get lost when the eraseblock containing them is garbage
> collected -- because they'll be treated like deletion dirents, which
> only need to remain on the medium for as long as the _real_ dirents
> which they exist to kill.
>
> This completely untested patch addresses some of it.

I think you are right.  Thanks!  I will add JFFS2 to my test suite
before the next release.  Right now I am testing mostly on UML, which
doesn't support the RAM-based MTD emulator as far I can tell.

> The other thing to verify is the three places in dir.c which check
> whether whiteout/rmdir/rename should return -ENOTEMPTY. Those all do so
> by checking whether the directory in question has any dirents with
> fd->ino != 0 -- i.e. does it contain any _real_ dirents, or only the
> deletion markers for dead stuff.
> 
> So that will now be _allowing_ you to remove a directory which contains
> whiteouts, since you haven't changed the test. Is that intentional? It
> seems sane at first glance.

Yes, you should be able to remove a directory which contains only
union mount-level whiteouts.

-VAL

> diff --git a/fs/jffs2/build.c b/fs/jffs2/build.c
> index c5e1450..4dc883f 100644
> --- a/fs/jffs2/build.c
> +++ b/fs/jffs2/build.c
> @@ -217,8 +217,9 @@ static void jffs2_build_remove_unlinked_inode(struct jffs2_sb_info *c,
>  			ic->scan_dents = fd->next;
>  
>  			if (!fd->ino) {
> -				/* It's a deletion dirent. Ignore it */
> -				dbg_fsbuild("child \"%s\" is a deletion dirent, skipping...\n", fd->name);
> +				dbg_fsbuild("child \"%s\" is a %s, skipping...\n",
> +					    fd->name,
> +					    (fd->type == DT_WHT)?"whiteout":"deletion dirent");
>  				jffs2_free_full_dirent(fd);
>  				continue;
>  			}
> diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
> index 090c556..7f5afbb 100644
> --- a/fs/jffs2/gc.c
> +++ b/fs/jffs2/gc.c
> @@ -516,7 +516,7 @@ static int jffs2_garbage_collect_live(struct jffs2_sb_info *c,  struct jffs2_era
>  			break;
>  	}
>  
> -	if (fd && fd->ino) {
> +	if (fd && (fd->ino || fd->type == DT_WHT)) {
>  		ret = jffs2_garbage_collect_dirent(c, jeb, f, fd);
>  	} else if (fd) {
>  		ret = jffs2_garbage_collect_deletion_dirent(c, jeb, f, fd);
> @@ -895,7 +895,7 @@ static int jffs2_garbage_collect_deletion_dirent(struct jffs2_sb_info *c, struct
>  				continue;
>  
>  			/* If the name length doesn't match, or it's another deletion dirent, skip */
> -			if (rd->nsize != name_len || !je32_to_cpu(rd->ino))
> +			if (rd->nsize != name_len || (!je32_to_cpu(rd->ino) && rd->type != DT_WHT))
>  				continue;
>  
>  			/* OK, check the actual name now */
> diff --git a/fs/jffs2/write.c b/fs/jffs2/write.c
> index ca29440..bcd4b86 100644
> --- a/fs/jffs2/write.c
> +++ b/fs/jffs2/write.c
> @@ -629,8 +629,9 @@ int jffs2_do_unlink(struct jffs2_sb_info *c, struct jffs2_inode_info *dir_f,
>  					printk(KERN_WARNING "Deleting inode #%u with active dentry \"%s\"->ino #%u\n",
>  					       dead_f->inocache->ino, fd->name, fd->ino);
>  				} else {
> -					D1(printk(KERN_DEBUG "Removing deletion dirent for \"%s\" from dir ino #%u\n",
> -						fd->name, dead_f->inocache->ino));
> +					D1(printk(KERN_DEBUG "Removing %s for \"%s\" from dir ino #%u\n",
> +						  (fd->type == DT_WHT)?"whiteout":"deletion dirent",
> +						  fd->name, dead_f->inocache->ino));
>  				}
>  				if (fd->raw)
>  					jffs2_mark_node_obsolete(c, fd->raw);
> 
> 
> -- 
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse@intel.com                              Intel Corporation
>