linux-ext4 - Re: [PATCH] ext4: shrink directory when last block is empty

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190125051227.GI8785@mit.edu>
Date:   Fri, 25 Jan 2019 00:12:27 -0500
From:   "Theodore Y. Ts'o" <tytso@....edu>
To:     <harshadshirwadkar@...il.com>
CC:     <umka@...udlinux.com>, <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: shrink directory when last block is empty

When you send a new version of the patch, it's good add [PATCH -v2] to
the subject prefix.  What I will usually do is run:

	rm -rf /tmp/p; git format-patch -o /tmp/p -1

and then edit the resulting patch in /tmp/p/0001-* before sending it
out via:

	git send-email --to=linux-ext4 --in-reply-to=<msgid> /tmp/p/*"

where I have an the following entry in ~/.mail_aliases:

alias linux-ext4 Ext4 Developers List <linux-ext4@...r.kernel.org>

Or you can use the subject-prefix option for git send-email and
git-format-patch.  I usually call git format-patch explicitly, since
if you are going to comments explaining what changed between the v1
and v2 patch (after the --- line), I'll needed to do that anyway.

> @@ -380,6 +381,7 @@ static void ext4_dirent_csum_set(struct inode *inode,
>  					   (void *)t - (void *)dirent);
>  }
>  
> +
>  int ext4_handle_dirty_dirent_node(handle_t *handle,
>  				  struct inode *inode,
>  				  struct buffer_head *bh)

Extra blank line added above?

> @@ -797,7 +799,7 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
>  	dxtrace(printk("Look up %x", hash));
>  	while (1) {
>  		count = dx_get_count(entries);
> -		if (!count || count > dx_get_limit(entries)) {
> +		if (count > dx_get_limit(entries)) {
>  			ext4_warning_inode(dir,
>  					   "dx entry: count %u beyond limit %u",
>  					   count, dx_get_limit(entries));

This was added because ext4_dx_delete_entry() can end up leaving an
interior node with no entries, right?  So this change stops the
warning from triggering.  The problem is after the warning, we fall
back to a brute-force linear search of the directory; and this will
happen if the file system is subsequently mounted on an old kernel
that doesn't have this change.

We have two choices here.  The first to simply not remove the last
directory block if it will result in an empty interior node.  That's
not particularly satisfying, but it's probably the simplest approach.
The other thing we can do is to try to remove the interior node; but
that gets tricky, since we now have to edit the parent node (and if
there is no parent node, handle the case of the now-completely empty
directory).

We also have to figure out what to do with that interior node.  We
can't just deallocate it, since that would leave a hole in the
directory and that could trigger the ext4_error_inode() call in
__ext4_read_dirblock().  So we would need to leave it as an "fake"
interior node which looks like an empty directory entry (in case of
the fallback to linear search option), but with soomething that makes
it clear that it is an orphaned interior node that can get garbage
collected or reused as a leaf block later.

The second choice is the right one, but it's more complicated.  So we
may want to leave that to a future patch, and keep this change small
and simple.

> @@ -1309,6 +1347,14 @@ int ext4_search_dir(struct buffer_head *bh, char *search_buf, int buf_size,
>  	return 0;
>  }
>  
> +static int is_empty_dirent_block(struct inode *dir, struct buffer_head *bh)
> +{
> +	struct ext4_dir_entry_2 *de = (struct ext4_dir_entry_2 *)bh->b_data;
> +
> +	return (ext4_rec_len_from_disk(de->rec_len, dir->i_sb->s_blocksize)
> +		== dir->i_sb->s_blocksize);
> +}
> +

You should also check to make sure de->inode is zero.

> @@ -1502,6 +1550,10 @@ static struct buffer_head * ext4_dx_find_entry(struct inode *dir,
>  	frame = dx_probe(fname, dir, NULL, frames);
>  	if (IS_ERR(frame))
>  		return (struct buffer_head *) frame;
> +	if (dx_frame) {
> +		*dx_frame = *frame;
> +		get_bh(dx_frame->bh);
> +	}

This should happen at the end of ext4_dx_find_entry(), and only if it
returns success.  On an error case, we should not fill in dx_frame and
bump the refcount on dx_frame->bh.  Otherwise, unless the callers are
very careful to remember to remember to call brelse(dx_frame->bh) in
the error case, or else we'll have a buffer head leak.

Module these issues, it looks good!  What sort of testing have you
done with this patch?  I recommend using gce-xfstests -g quick before
you send out a patch for review, just to save yourself (and me!) time.

Thanks,

					- Ted