2007-05-29 05:48:58

by Aneesh Kumar K.V

[permalink] [raw]
Subject: [PATCH] update ext4-nanosecond-patch comments

Also can we have a description of why s_{min, want}_extra_isize
fields are added in the commit message ?



diff --git a/ext4-nanosecond-patch b/ext4-nanosecond-patch
index ceaf339..02d00b7 100644
--- a/ext4-nanosecond-patch
+++ b/ext4-nanosecond-patch
@@ -1,8 +1,8 @@
This patch is a spinoff of the old nanosecond patches.

It includes some cleanups and addition of a creation timestamp. The
-EXT3_FEATURE_RO_COMPAT_EXTRA_ISIZE flag has also been added along with
-s_{min, want}_extra_isize fields in struct ext3_super_block.
+EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE flag has also been added along with
+s_{min, want}_extra_isize fields in struct ext4_super_block.

Signed-off-by: Andreas Dilger <[email protected]>
Signed-off-by: Kalpak Shah <[email protected]>


2007-05-29 06:34:46

by Kalpak Shah

[permalink] [raw]
Subject: Re: [PATCH] update ext4-nanosecond-patch comments

On Tue, 2007-05-29 at 11:18 +0530, Aneesh Kumar K.V wrote:
> Also can we have a description of why s_{min, want}_extra_isize
> fields are added in the commit message ?

The i_extra_isize for each inode should ideally be s_want_extra_isize
after inode expansion. If expansion by s_want_extra_isize is not
possible, then i_extra_isize must be atleast s_min_extra_isize.

Thanks,
Kalpak.

> diff --git a/ext4-nanosecond-patch b/ext4-nanosecond-patch
> index ceaf339..02d00b7 100644
> --- a/ext4-nanosecond-patch
> +++ b/ext4-nanosecond-patch
> @@ -1,8 +1,8 @@
> This patch is a spinoff of the old nanosecond patches.
>
> It includes some cleanups and addition of a creation timestamp. The
> -EXT3_FEATURE_RO_COMPAT_EXTRA_ISIZE flag has also been added along with
> -s_{min, want}_extra_isize fields in struct ext3_super_block.
> +EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE flag has also been added along with
> +s_{min, want}_extra_isize fields in struct ext4_super_block.
>
> Signed-off-by: Andreas Dilger <[email protected]>
> Signed-off-by: Kalpak Shah <[email protected]>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2007-05-29 08:18:49

by Aneesh Kumar K.V

[permalink] [raw]
Subject: Re: [PATCH] update ext4-nanosecond-patch comments



Kalpak Shah wrote:
> On Tue, 2007-05-29 at 11:18 +0530, Aneesh Kumar K.V wrote:
>> Also can we have a description of why s_{min, want}_extra_isize
>> fields are added in the commit message ?
>
> The i_extra_isize for each inode should ideally be s_want_extra_isize
> after inode expansion. If expansion by s_want_extra_isize is not
> possible, then i_extra_isize must be atleast s_min_extra_isize.
>
>

My point was that the commit message of ext4-nanosecond-patch should
explain the usage scenario for these variables. We may want to move the
commit message of ext4_expand_inode_extra_isize.patch reworded to this.


BTW i also think that adding s_{min, want}_extra_isize can be a separate
patch altogether.


-aneesh

2007-05-29 11:53:29

by Andreas Dilger

[permalink] [raw]
Subject: Re: [PATCH] update ext4-nanosecond-patch comments

On May 29, 2007 13:48 +0530, Aneesh Kumar K.V wrote:
> Kalpak Shah wrote:
> >On Tue, 2007-05-29 at 11:18 +0530, Aneesh Kumar K.V wrote:
> >>Also can we have a description of why s_{min, want}_extra_isize
> >>fields are added in the commit message ?
> >
> >The i_extra_isize for each inode should ideally be s_want_extra_isize
> >after inode expansion. If expansion by s_want_extra_isize is not
> >possible, then i_extra_isize must be atleast s_min_extra_isize.
>
> My point was that the commit message of ext4-nanosecond-patch should
> explain the usage scenario for these variables. We may want to move the
> commit message of ext4_expand_inode_extra_isize.patch reworded to this.
>
> BTW i also think that adding s_{min, want}_extra_isize can be a separate
> patch altogether.

When the nanosecond timestamp extension was first proposed, the requirement
from Ted and Stephen were that s_min_extra_isize was a requirement. Otherwise
it would be possible to have a filesystem where the timestamps are going
backward on some files due to MOST of the files supporting ns timestamps,
but some with full EAs having to truncate the ns part away.

Now, this might not be critical for some users, but for others it can be.
Since this functionality is all here there isn't any reason to move it to
a separate patch. The same fields will be important for the inode version
also.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

2007-05-29 12:28:00

by Aneesh Kumar K.V

[permalink] [raw]
Subject: Re: [PATCH] update ext4-nanosecond-patch comments



Andreas Dilger wrote:
> On May 29, 2007 13:48 +0530, Aneesh Kumar K.V wrote:
>>
>
> When the nanosecond timestamp extension was first proposed, the requirement
> from Ted and Stephen were that s_min_extra_isize was a requirement. Otherwise
> it would be possible to have a filesystem where the timestamps are going
> backward on some files due to MOST of the files supporting ns timestamps,
> but some with full EAs having to truncate the ns part away.
>
> Now, this might not be critical for some users, but for others it can be.
> Since this functionality is all here there isn't any reason to move it to
> a separate patch. The same fields will be important for the inode version
> also.
>

That is why i was thinking it should not be buried in the nanosecond
patch. Since there are multiple features depending on this, a nice patch
list would be

Add extra fields to superblock to take care of enabling feature after
file system creation

Add nano second feature

Add inode version feature

etc.

If wanted, i can attempt to split the patch as above. Let me know. If we
don't think the above is important I would say we should at least move
some of the commit message found in expand_inode_extra_isize.patch that
explains the usage to the patch that introduce these fields (nano second
patch ).

-aneesh