Message-Id: <6FFCEA37-004E-4675-AB03-171DAB91C80F@dilger.ca>
Date:   Thu, 30 Nov 2017 15:17:00 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Artem Blagodarenko <artem.blagodarenko@...il.com>
Cc:     linux-ext4 <linux-ext4@...r.kernel.org>,
        Yang Sheng <yang.sheng@...el.com>
Subject: Re: [PATCH v3 1/3] ext4: Removes static definition of dx_root struct

On Nov 30, 2017, at 8:17 AM, Artem Blagodarenko <artem.blagodarenko@...il.com> wrote:
> 
> From: Yang Sheng <yang.sheng@...el.com>
> 
> Removes static definition of dx_root struct. so that "." and ".." dirent
> can have extra data. This patch does not change any functionality but is
> required for ext4_data_in_dirent patch.
> 
> Signed-off-by: Yang Sheng <yang.sheng@...el.com>
> Signed-off-by: Artem Blagodarenko <artem.blagodarenko@...il.com>
> ---
> fs/ext4/namei.c | 130 +++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 71 insertions(+), 59 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index c1cf020d1889..7c649cf2b630 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -193,23 +193,13 @@ struct dx_entry
>  * dirent the two low bits of the hash version will be zero.  Therefore, the
>  * hash version mod 4 should never be 0.  Sincerely, the paranoia department.
>  */
> -
> -struct dx_root
> +struct dx_root_info
> {
> -	struct fake_dirent dot;
> -	char dot_name[4];
> -	struct fake_dirent dotdot;
> -	char dotdot_name[4];
> -	struct dx_root_info
> -	{
> -		__le32 reserved_zero;
> -		u8 hash_version;
> -		u8 info_length; /* 8 */
> -		u8 indirect_levels;
> -		u8 unused_flags;
> -	}
> -	info;
> -	struct dx_entry	entries[0];
> +	__le32 reserved_zero;
> +	u8 hash_version;
> +	u8 info_length; /* 8 */
> +	u8 indirect_levels;
> +	u8 unused_flags;
> };
> 
> struct dx_node
> @@ -521,6 +511,17 @@ static inline void dx_set_block(struct dx_entry *entry, ext4_lblk_t value)
> 	entry->block = cpu_to_le32(value);
> }
> 
> +struct dx_root_info *dx_get_dx_info(struct ext4_dir_entry_2 *de)
> +{
> +	/* get dotdot first */
> +	de = (struct ext4_dir_entry_2 *)((char *)de + EXT4_DIR_NAME_LEN(1));
> +
> +	/* dx root info is after dotdot entry */
> +	de = (struct ext4_dir_entry_2 *)((char *)de + EXT4_DIR_NAME_LEN(2));
> +
> +	return (struct dx_root_info *)de;
> +}
> +
> static inline unsigned dx_get_hash(struct dx_entry *entry)
> {
> 	return le32_to_cpu(entry->hash);
> @@ -744,17 +745,17 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
> 	if (IS_ERR(frame->bh))
> 		return (struct dx_frame *) frame->bh;
> 
> -	root = (struct dx_root *) frame->bh->b_data;
> -	if (root->info.hash_version != DX_HASH_TEA &&
> -	    root->info.hash_version != DX_HASH_HALF_MD4 &&
> -	    root->info.hash_version != DX_HASH_LEGACY) {
> -		ext4_warning_inode(dir, "Unrecognised inode hash code %u",
> -				   root->info.hash_version);
> +	info = dx_get_dx_info((struct ext4_dir_entry_2 *)frame->bh->b_data);
> +	if (info->hash_version != DX_HASH_TEA &&
> +	    info->hash_version != DX_HASH_HALF_MD4 &&
> +	    info->hash_version != DX_HASH_LEGACY) {
> +		ext4_warning(dir->i_sb, "inode #%lu: unrecognised hash code %u",
> +			     dir->i_ino, info->hash_version);

This is probably a conflict from the older version of the dx_root patch with
newer error messages.  The new ext4_warning_inode() message should be kept,
and just the "root->info." usage replaced by "info->".

> @@ -762,18 +763,18 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
> 
> -	if (root->info.unused_flags & 1) {
> -		ext4_warning_inode(dir, "Unimplemented hash flags: %#06x",
> -				   root->info.unused_flags);
> +	if (info->unused_flags & 1) {
> +		ext4_warning_inode(dir,
> +				"inode #%lu: Unimplemented hash flags: %#06x",
> +				dir->i_ino, info->unused_flags);

Similarly, the ext4_warning_inode() message already prints "inode #%lu: " at
the start, so the old message should be kept and just replace "root->info.".

> 	if (indirect >= ext4_dir_htree_level(dir->i_sb)) {
> 		ext4_warning(dir->i_sb,
> -			     "Directory (ino: %lu) htree depth %#06x exceed"
> -			     "supported value", dir->i_ino,
> -			     ext4_dir_htree_level(dir->i_sb));
> +			     "inode #%lu unimplemented hash depth %#06x",
> +			     dir->i_ino, ext4_dir_htree_level(dir->i_sb));

This should also be using ext4_warning_inode() and drop the "inode #%lu"
at the start, leaving "directory htree depth %u exceeds supported value %u",
and also print ext4_dir_htree_level(dir->i_sb) at the end.  It isn't clear
why the depth was printed in hex previously, that doesn't make sense.

> @@ -781,14 +782,17 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
> 
> 	if (dx_get_limit(entries) != dx_root_limit(dir,

It would make sense to move "dx_root_limit(dir," to the next line, aligned
after 'if (', so that it is not wrapping so close to the end.

> -						   root->info.info_length)) {
> -		ext4_warning_inode(dir, "dx entry: limit %u != root limit %u",
> -				   dx_get_limit(entries),
> -				   dx_root_limit(dir, root->info.info_length));
> +				(struct ext4_dir_entry_2 *) frame->bh->b_data,
> +				info->info_length)) {
> +		ext4_warning(dir->i_sb, "dx entry: limit != root limit "
> +			     "inode #%lu: dx entry: limit %u != root limit %u",
> +			     dir->i_ino, dx_get_limit(entries),
> +			     dx_root_limit(dir, (struct ext4_dir_entry_2 *)
> +					   frame->bh->b_data,
> +					   info->info_length));

Use the old message with ext4_warning_inode(), which doesn't have the
duplicate "dx entry:" part, and doesn't need the "inode #%lu: " anymore.

Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)

Powered by blists - more mailing lists