diff mbox series

[9/9] common/attr: Reduce MAX_ATTRS to leave some overhead for 64K blocksize

Message ID f23e6788b958849ec9c1fb7fed0081e58c02a13a.1623651783.git.riteshh@linux.ibm.com
State Not Applicable
Headers show
Series 64K blocksize related fixes | expand

Commit Message

Ritesh Harjani June 14, 2021, 6:28 a.m. UTC
Test generic/020 fails for ext4 with 64K blocksize. So increase some overhead
value to reduce the MAX_ATTRS so that it can accomodate for 64K blocksize.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 common/attr | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Darrick J. Wong June 30, 2021, 3:51 p.m. UTC | #1
On Mon, Jun 14, 2021 at 11:58:13AM +0530, Ritesh Harjani wrote:
> Test generic/020 fails for ext4 with 64K blocksize. So increase some overhead
> value to reduce the MAX_ATTRS so that it can accomodate for 64K blocksize.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>  common/attr | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/attr b/common/attr
> index d3902346..e8661d80 100644
> --- a/common/attr
> +++ b/common/attr
> @@ -260,7 +260,7 @@ xfs|udf|pvfs2|9p|ceph|nfs)
>  	# Assume max ~1 block of attrs
>  	BLOCK_SIZE=`_get_block_size $TEST_DIR`
>  	# user.attribute_XXX="value.XXX" is about 32 bytes; leave some overhead
> -	let MAX_ATTRS=$BLOCK_SIZE/40
> +	let MAX_ATTRS=$BLOCK_SIZE/48

50% is quite a lot of overhead; maybe we should special-case this?

--D

>  esac
>  
>  export MAX_ATTRS
> -- 
> 2.31.1
>
Theodore Ts'o June 30, 2021, 7:20 p.m. UTC | #2
On Wed, Jun 30, 2021 at 08:51:50AM -0700, Darrick J. Wong wrote:
> On Mon, Jun 14, 2021 at 11:58:13AM +0530, Ritesh Harjani wrote:
> > Test generic/020 fails for ext4 with 64K blocksize. So increase some overhead
> > value to reduce the MAX_ATTRS so that it can accomodate for 64K blocksize.
> > 
> > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > ---
> >  common/attr | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/common/attr b/common/attr
> > index d3902346..e8661d80 100644
> > --- a/common/attr
> > +++ b/common/attr
> > @@ -260,7 +260,7 @@ xfs|udf|pvfs2|9p|ceph|nfs)
> >  	# Assume max ~1 block of attrs
> >  	BLOCK_SIZE=`_get_block_size $TEST_DIR`
> >  	# user.attribute_XXX="value.XXX" is about 32 bytes; leave some overhead
> > -	let MAX_ATTRS=$BLOCK_SIZE/40
> > +	let MAX_ATTRS=$BLOCK_SIZE/48
> 
> 50% is quite a lot of overhead; maybe we should special-case this?

The problem is that 32 bytes is an underestimate when i > 99 for
user.attribute_$i=value_$i.  And with a 4k blocksize, MAX_ATTRS =
4096 / 40 = 102.

The exact calculation for ext4 is:

fixed_block_overhead = 32
fixed_entry_overhead = 16
max_attr = (block_size - fixed_block_overhead) /
	(fixed_entry_overhead + round_up(len(attr_name), 4) +
	 round_up(len(value), 4))

For 4k blocksizes, most of the attributes have an attr_name of
"attribute_NN" which is 8, and "value_NN" which is 12.

But for larger block sizes, we start having extended attributes of the
form "attribute_NNN" or "attribute_NNNN", and "value_NNN" and
"value_NNNN", which causes the round(len(..), 4) to jump up by 4
bytes.  So round_up(len(attr_name, 4)) becomes 12 instead of 8, and
round_up(len(value, 4)) becomes 16 instead of 12.  So:

	max_attrs = (block_size - 32) / (16 + 12 + 16)
or
	max_attrs = (block_size - 32) / 44

instead of:

	max_attrs = (block_size - 32) / (16 + 8 + 12)
or
	max_attrs = (block_size - 32) / 36

So special casing things for block sizes > 4k may very well make
sense.  Perhaps it's even worth it to put in an ext[234] specific,
exalc calculation for MAX_ATTRS in common/attr.

Cheers,

						- Ted
Ritesh Harjani July 9, 2021, 5:12 a.m. UTC | #3
On 21/06/30 03:20PM, Theodore Ts'o wrote:
> On Wed, Jun 30, 2021 at 08:51:50AM -0700, Darrick J. Wong wrote:
> > On Mon, Jun 14, 2021 at 11:58:13AM +0530, Ritesh Harjani wrote:
> > > Test generic/020 fails for ext4 with 64K blocksize. So increase some overhead
> > > value to reduce the MAX_ATTRS so that it can accomodate for 64K blocksize.
> > >
> > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > > ---
> > >  common/attr | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/common/attr b/common/attr
> > > index d3902346..e8661d80 100644
> > > --- a/common/attr
> > > +++ b/common/attr
> > > @@ -260,7 +260,7 @@ xfs|udf|pvfs2|9p|ceph|nfs)
> > >  	# Assume max ~1 block of attrs
> > >  	BLOCK_SIZE=`_get_block_size $TEST_DIR`
> > >  	# user.attribute_XXX="value.XXX" is about 32 bytes; leave some overhead
> > > -	let MAX_ATTRS=$BLOCK_SIZE/40
> > > +	let MAX_ATTRS=$BLOCK_SIZE/48
> >
> > 50% is quite a lot of overhead; maybe we should special-case this?
>
> The problem is that 32 bytes is an underestimate when i > 99 for
> user.attribute_$i=value_$i.  And with a 4k blocksize, MAX_ATTRS =
> 4096 / 40 = 102.
>
> The exact calculation for ext4 is:
>
> fixed_block_overhead = 32
> fixed_entry_overhead = 16
> max_attr = (block_size - fixed_block_overhead) /
> 	(fixed_entry_overhead + round_up(len(attr_name), 4) +
> 	 round_up(len(value), 4))
>
> For 4k blocksizes, most of the attributes have an attr_name of
> "attribute_NN" which is 8, and "value_NN" which is 12.
>
> But for larger block sizes, we start having extended attributes of the
> form "attribute_NNN" or "attribute_NNNN", and "value_NNN" and
> "value_NNNN", which causes the round(len(..), 4) to jump up by 4
> bytes.  So round_up(len(attr_name, 4)) becomes 12 instead of 8, and
> round_up(len(value, 4)) becomes 16 instead of 12.  So:
>
> 	max_attrs = (block_size - 32) / (16 + 12 + 16)
> or
> 	max_attrs = (block_size - 32) / 44
>
> instead of:
>
> 	max_attrs = (block_size - 32) / (16 + 8 + 12)
> or
> 	max_attrs = (block_size - 32) / 36

Thanks for the indepth details. Yes, in my testing as well it was coming to be
around ~44%. But to be safe I chose 50%.
I verified from the code as well. We do have 32 bytes of overhead per block for
the the header. And per entry overhead of 16 bytes. The rounding happens for
both name (EXT4_XATTR_LEN) and value (EXT4_XATTR_SIZE) of attr.

Perhaps, it will be helpful if we update above info in ext4 documentation as
well. In that the rounding off is only mentioned for value and not for name
length.

>
> So special casing things for block sizes > 4k may very well make
> sense.  Perhaps it's even worth it to put in an ext[234] specific,
> exalc calculation for MAX_ATTRS in common/attr.

yes, will make these changes for ext[234] specific in common/attr.

-ritesh
diff mbox series

Patch

diff --git a/common/attr b/common/attr
index d3902346..e8661d80 100644
--- a/common/attr
+++ b/common/attr
@@ -260,7 +260,7 @@  xfs|udf|pvfs2|9p|ceph|nfs)
 	# Assume max ~1 block of attrs
 	BLOCK_SIZE=`_get_block_size $TEST_DIR`
 	# user.attribute_XXX="value.XXX" is about 32 bytes; leave some overhead
-	let MAX_ATTRS=$BLOCK_SIZE/40
+	let MAX_ATTRS=$BLOCK_SIZE/48
 esac
 
 export MAX_ATTRS