[PATCH] erofs-utils: mkfs: Use extended inodes when ctime is set

[PATCH] erofs-utils: mkfs: Use extended inodes when ctime is set

[David Anderson via Linux-erofs]

Currently ctime is effectively ignored because most inodes are compact.
If ctime was set, and it's different from the build time, then extended
inodes should be used instead.

To guarantee that timestamps do not cause extended inodes, a fixed
timestamp should be used (-T).

Change-Id: I3d2e5c5ffe2bdcabea6791b5def5973b507aa316
Signed-off-by: David Anderson <dvander@google.com>
---
 lib/inode.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/inode.c b/lib/inode.c
index f0a71a8..6f36c09 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -730,6 +730,10 @@ static bool erofs_should_use_inode_extended(struct erofs_inode *inode)
 		return true;
 	if (inode->i_nlink > USHRT_MAX)
 		return true;
+	if (inode->i_ctime != sbi.build_time ||
+	    inode->i_ctime_nsec != sbi.build_time_nsec) {
+		return true;
+	}
 	return false;
 }
 
-- 
2.35.1.574.g5d30c73bfb-goog

Re: [PATCH] erofs-utils: mkfs: Use extended inodes when ctime is set

[Gao Xiang]

Hi David,

On Tue, Mar 01, 2022 at 04:10:37AM +0000, David Anderson via Linux-erofs wrote:
> Currently ctime is effectively ignored because most inodes are compact.
> If ctime was set, and it's different from the build time, then extended
> inodes should be used instead.
> 
> To guarantee that timestamps do not cause extended inodes, a fixed
> timestamp should be used (-T).

Thanks for the patch.

How about introducing some option like '-E preserve-time' since
compact inodes are mainly used for reproducible builds, which I think
per-file timestamp is useless...

Also after I checked ociv2-brainstorm, they'd like to avoid timestamp as
well, which can be effective used by EROFS compact inodes...

https://hackmd.io/@cyphar/ociv2-brainstorm

Thanks,
Gao Xiang

> 
> Change-Id: I3d2e5c5ffe2bdcabea6791b5def5973b507aa316
> Signed-off-by: David Anderson <dvander@google.com>
> ---
>  lib/inode.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/inode.c b/lib/inode.c
> index f0a71a8..6f36c09 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -730,6 +730,10 @@ static bool erofs_should_use_inode_extended(struct erofs_inode *inode)
>  		return true;
>  	if (inode->i_nlink > USHRT_MAX)
>  		return true;
> +	if (inode->i_ctime != sbi.build_time ||
> +	    inode->i_ctime_nsec != sbi.build_time_nsec) {
> +		return true;
> +	}
>  	return false;
>  }
>  
> -- 
> 2.35.1.574.g5d30c73bfb-goog

Re: [PATCH] erofs-utils: mkfs: Use extended inodes when ctime is set

[David Anderson via Linux-erofs]

On Mon, Feb 28, 2022 at 9:17 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> How about introducing some option like '-E preserve-time' since
> compact inodes are mainly used for reproducible builds, which I think
> per-file timestamp is useless...
>
> Also after I checked ociv2-brainstorm, they'd like to avoid timestamp as
> well, which can be effective used by EROFS compact inodes...

Sounds good, I'll post a new patch with this option.

Best,

-David

Re: [PATCH] erofs-utils: mkfs: Use extended inodes when ctime is set

[Gao Xiang]

On Mon, Feb 28, 2022 at 10:06:33PM -0800, David Anderson wrote:
> On Mon, Feb 28, 2022 at 9:17 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> > How about introducing some option like '-E preserve-time' since
> > compact inodes are mainly used for reproducible builds, which I think
> > per-file timestamp is useless...
> >
> > Also after I checked ociv2-brainstorm, they'd like to avoid timestamp as
> > well, which can be effective used by EROFS compact inodes...
> 
> Sounds good, I'll post a new patch with this option.

(Or maybe just "--preserve-mtime" if it's supposed to be used frequently in
 specific use cases...)

Thanks,
Gao Xiang

> 
> Best,
> 
> -David

Re: [PATCH] erofs-utils: mkfs: Use extended inodes when ctime is set

[David Anderson via Linux-erofs]

On Mon, Feb 28, 2022 at 10:14 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
>
>
> (Or maybe just "--preserve-mtime" if it's supposed to be used frequently in
>  specific use cases...)

Looking at this again - is a new flag needed?

If -T is specified, the build will be reproducible. All timestamps
will be equal, and we'll never force an extended inode due to
mismatching timestamps. The patch has no effect in this case.

If -T is *not* specified, inode mtime *could* be reproducible
(depending on how the user creates the files), but sbi.build_time
isn't. Any of these inodes that gets the compact layout will use the
unreproducible build time instead. So, switching to extended inodes
here strictly improves reproducibility and determinism.

That said, it is a change in behavior, so maybe that warrants a flag anyway.

For what it's worth, in AOSP, "-T" is specified for production builds.
Non-production builds have "mtime" preserved for pushing incremental
changes.

Best,

-David

Re: [PATCH] erofs-utils: mkfs: Use extended inodes when ctime is set

[Gao Xiang]

Hi David,

On Thu, Mar 10, 2022 at 07:53:07PM -0800, David Anderson via Linux-erofs wrote:
> On Mon, Feb 28, 2022 at 10:14 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> >
> >
> > (Or maybe just "--preserve-mtime" if it's supposed to be used frequently in
> >  specific use cases...)
> 
> Looking at this again - is a new flag needed?
> 
> If -T is specified, the build will be reproducible. All timestamps
> will be equal, and we'll never force an extended inode due to
> mismatching timestamps. The patch has no effect in this case.
> 
> If -T is *not* specified, inode mtime *could* be reproducible
> (depending on how the user creates the files), but sbi.build_time
> isn't. Any of these inodes that gets the compact layout will use the
> unreproducible build time instead. So, switching to extended inodes
> here strictly improves reproducibility and determinism.

Yeah, I know what's your main point here. Ok, that's fine we
could change the current behavior. Yet I still think the current
behavior may benefit some use cases (e.g. metadata size is sensible,
we need to avoid extended inodes no matter per-filetimestamp is)..

So instead, how about introducing --ignore-mtime instead for the
current behavior, and change the default behavior into using extended
inodes?

> 
> That said, it is a change in behavior, so maybe that warrants a flag anyway.
> 
> For what it's worth, in AOSP, "-T" is specified for production builds.
> Non-production builds have "mtime" preserved for pushing incremental
> changes.

Thanks for the information!

Thanks,
Gao Xiang

> 
> Best,
> 
> -David

Re: [PATCH] erofs-utils: mkfs: Use extended inodes when ctime is set

[David Anderson via Linux-erofs]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

On Thu, Mar 10, 2022 at 9:24 PM Gao Xiang <hsiangkao@linux.alibaba.com>
wrote:

> Yeah, I know what's your main point here. Ok, that's fine we
> could change the current behavior. Yet I still think the current
> behavior may benefit some use cases (e.g. metadata size is sensible,
> we need to avoid extended inodes no matter per-filetimestamp is)..
>
> So instead, how about introducing --ignore-mtime instead for the
> current behavior, and change the default behavior into using extended
> inodes?
>
>
Sounds good to me, I'll make this change.

[-- Attachment #2: Type: text/html, Size: 915 bytes --]