linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] erofs: derive atime instead of leaving it empty
       [not found] <20201031195102.21221-1-hsiangkao.ref@aol.com>
@ 2020-10-31 19:51 ` Gao Xiang
  2020-11-03  2:50   ` Gao Xiang
  2020-11-04  0:57   ` Chao Yu
  0 siblings, 2 replies; 5+ messages in thread
From: Gao Xiang @ 2020-10-31 19:51 UTC (permalink / raw)
  To: linux-erofs, Chao Yu, Chao Yu; +Cc: LKML, Gao Xiang, nl6720, stable

From: Gao Xiang <hsiangkao@redhat.com>

EROFS has _only one_ ondisk timestamp (ctime is currently
documented and recorded, we might also record mtime instead
with a new compat feature if needed) for each extended inode
since EROFS isn't mainly for archival purposes so no need to
keep all timestamps on disk especially for Android scenarios
due to security concerns. Also, romfs/cramfs don't have their
own on-disk timestamp, and squashfs only records mtime instead.

Let's also derive access time from ondisk timestamp rather than
leaving it empty, and if mtime/atime for each file are really
needed for specific scenarios as well, we can also use xattrs
to record them then.

Reported-by: nl6720 <nl6720@gmail.com>
[ Gao Xiang: It'd be better to backport for user-friendly concern. ]
Fixes: 431339ba9042 ("staging: erofs: add inode operations")
Cc: stable <stable@vger.kernel.org> # 4.19+
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/erofs/inode.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
index 139d0bed42f8..3e21c0e8adae 100644
--- a/fs/erofs/inode.c
+++ b/fs/erofs/inode.c
@@ -107,11 +107,9 @@ static struct page *erofs_read_inode(struct inode *inode,
 		i_gid_write(inode, le32_to_cpu(die->i_gid));
 		set_nlink(inode, le32_to_cpu(die->i_nlink));
 
-		/* ns timestamp */
-		inode->i_mtime.tv_sec = inode->i_ctime.tv_sec =
-			le64_to_cpu(die->i_ctime);
-		inode->i_mtime.tv_nsec = inode->i_ctime.tv_nsec =
-			le32_to_cpu(die->i_ctime_nsec);
+		/* extended inode has its own timestamp */
+		inode->i_ctime.tv_sec = le64_to_cpu(die->i_ctime);
+		inode->i_ctime.tv_nsec = le32_to_cpu(die->i_ctime_nsec);
 
 		inode->i_size = le64_to_cpu(die->i_size);
 
@@ -149,11 +147,9 @@ static struct page *erofs_read_inode(struct inode *inode,
 		i_gid_write(inode, le16_to_cpu(dic->i_gid));
 		set_nlink(inode, le16_to_cpu(dic->i_nlink));
 
-		/* use build time to derive all file time */
-		inode->i_mtime.tv_sec = inode->i_ctime.tv_sec =
-			sbi->build_time;
-		inode->i_mtime.tv_nsec = inode->i_ctime.tv_nsec =
-			sbi->build_time_nsec;
+		/* use build time for compact inodes */
+		inode->i_ctime.tv_sec = sbi->build_time;
+		inode->i_ctime.tv_nsec = sbi->build_time_nsec;
 
 		inode->i_size = le32_to_cpu(dic->i_size);
 		if (erofs_inode_is_data_compressed(vi->datalayout))
@@ -167,6 +163,11 @@ static struct page *erofs_read_inode(struct inode *inode,
 		goto err_out;
 	}
 
+	inode->i_mtime.tv_sec = inode->i_ctime.tv_sec;
+	inode->i_atime.tv_sec = inode->i_ctime.tv_sec;
+	inode->i_mtime.tv_nsec = inode->i_ctime.tv_nsec;
+	inode->i_atime.tv_nsec = inode->i_ctime.tv_nsec;
+
 	if (!nblks)
 		/* measure inode.i_blocks as generic filesystems */
 		inode->i_blocks = roundup(inode->i_size, EROFS_BLKSIZ) >> 9;
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] erofs: derive atime instead of leaving it empty
  2020-10-31 19:51 ` [PATCH] erofs: derive atime instead of leaving it empty Gao Xiang
@ 2020-11-03  2:50   ` Gao Xiang
  2020-11-03 15:58     ` Chao Yu
  2020-11-04  0:57   ` Chao Yu
  1 sibling, 1 reply; 5+ messages in thread
From: Gao Xiang @ 2020-11-03  2:50 UTC (permalink / raw)
  To: Chao Yu, Chao Yu; +Cc: linux-erofs, LKML, nl6720, stable

Hi Chao,

On Sun, Nov 01, 2020 at 03:51:02AM +0800, Gao Xiang wrote:
> From: Gao Xiang <hsiangkao@redhat.com>
> 
> EROFS has _only one_ ondisk timestamp (ctime is currently
> documented and recorded, we might also record mtime instead
> with a new compat feature if needed) for each extended inode
> since EROFS isn't mainly for archival purposes so no need to
> keep all timestamps on disk especially for Android scenarios
> due to security concerns. Also, romfs/cramfs don't have their
> own on-disk timestamp, and squashfs only records mtime instead.
> 
> Let's also derive access time from ondisk timestamp rather than
> leaving it empty, and if mtime/atime for each file are really
> needed for specific scenarios as well, we can also use xattrs
> to record them then.
> 
> Reported-by: nl6720 <nl6720@gmail.com>
> [ Gao Xiang: It'd be better to backport for user-friendly concern. ]
> Fixes: 431339ba9042 ("staging: erofs: add inode operations")
> Cc: stable <stable@vger.kernel.org> # 4.19+
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

May I ask for some extra free slots to review this patch plus 
[PATCH 1/4] of 
https://lore.kernel.org/r/20201022145724.27284-1-hsiangkao@aol.com

since it'd be also in linux-next for a while before sending out
to Linus. And the debugging messages may also be an annoying
thing for users.

Thanks a lot!

Thanks,
Gao Xiang


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] erofs: derive atime instead of leaving it empty
  2020-11-03  2:50   ` Gao Xiang
@ 2020-11-03 15:58     ` Chao Yu
  2020-11-03 17:04       ` Gao Xiang
  0 siblings, 1 reply; 5+ messages in thread
From: Chao Yu @ 2020-11-03 15:58 UTC (permalink / raw)
  To: Gao Xiang, Chao Yu; +Cc: linux-erofs, LKML, nl6720, stable

Hi Xiang,

On 2020-11-3 10:50, Gao Xiang wrote:
> Hi Chao,
>
> On Sun, Nov 01, 2020 at 03:51:02AM +0800, Gao Xiang wrote:
>> From: Gao Xiang <hsiangkao@redhat.com>
>>
>> EROFS has _only one_ ondisk timestamp (ctime is currently
>> documented and recorded, we might also record mtime instead
>> with a new compat feature if needed) for each extended inode
>> since EROFS isn't mainly for archival purposes so no need to
>> keep all timestamps on disk especially for Android scenarios
>> due to security concerns. Also, romfs/cramfs don't have their
>> own on-disk timestamp, and squashfs only records mtime instead.
>>
>> Let's also derive access time from ondisk timestamp rather than
>> leaving it empty, and if mtime/atime for each file are really
>> needed for specific scenarios as well, we can also use xattrs
>> to record them then.
>>
>> Reported-by: nl6720 <nl6720@gmail.com>
>> [ Gao Xiang: It'd be better to backport for user-friendly concern. ]
>> Fixes: 431339ba9042 ("staging: erofs: add inode operations")
>> Cc: stable <stable@vger.kernel.org> # 4.19+
>> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
>
> May I ask for some extra free slots to review this patch plus
> [PATCH 1/4] of
> https://lore.kernel.org/r/20201022145724.27284-1-hsiangkao@aol.com
>
> since it'd be also in linux-next for a while before sending out
> to Linus. And the debugging messages may also be an annoying
> thing for users.

Sorry for the delay review, will check the details tomorrow. :)

Thanks,

>
> Thanks a lot!
>
> Thanks,
> Gao Xiang
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] erofs: derive atime instead of leaving it empty
  2020-11-03 15:58     ` Chao Yu
@ 2020-11-03 17:04       ` Gao Xiang
  0 siblings, 0 replies; 5+ messages in thread
From: Gao Xiang @ 2020-11-03 17:04 UTC (permalink / raw)
  To: Chao Yu; +Cc: Chao Yu, linux-erofs, LKML, nl6720, stable

Hi Chao,

On Tue, Nov 03, 2020 at 11:58:42PM +0800, Chao Yu wrote:
> Hi Xiang,
> 
> On 2020-11-3 10:50, Gao Xiang wrote:
> > Hi Chao,
> > 
> > On Sun, Nov 01, 2020 at 03:51:02AM +0800, Gao Xiang wrote:
> > > From: Gao Xiang <hsiangkao@redhat.com>
> > > 
> > > EROFS has _only one_ ondisk timestamp (ctime is currently
> > > documented and recorded, we might also record mtime instead
> > > with a new compat feature if needed) for each extended inode
> > > since EROFS isn't mainly for archival purposes so no need to
> > > keep all timestamps on disk especially for Android scenarios
> > > due to security concerns. Also, romfs/cramfs don't have their
> > > own on-disk timestamp, and squashfs only records mtime instead.
> > > 
> > > Let's also derive access time from ondisk timestamp rather than
> > > leaving it empty, and if mtime/atime for each file are really
> > > needed for specific scenarios as well, we can also use xattrs
> > > to record them then.
> > > 
> > > Reported-by: nl6720 <nl6720@gmail.com>
> > > [ Gao Xiang: It'd be better to backport for user-friendly concern. ]
> > > Fixes: 431339ba9042 ("staging: erofs: add inode operations")
> > > Cc: stable <stable@vger.kernel.org> # 4.19+
> > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > 
> > May I ask for some extra free slots to review this patch plus
> > [PATCH 1/4] of
> > https://lore.kernel.org/r/20201022145724.27284-1-hsiangkao@aol.com
> > 
> > since it'd be also in linux-next for a while before sending out
> > to Linus. And the debugging messages may also be an annoying
> > thing for users.
> 
> Sorry for the delay review, will check the details tomorrow. :)

No problem, thanks! If we'd like to submit a -fixes pull request,
it'd be better not to be in the latter half of 5.10 rc... And
considering that it'd be stayed in linux-next for almost a week,
so yeah... (Only these 2 patches are considered for -fixes for now.)

Thanks.
Gao Xiang

> 
> Thanks,
> 
> > 
> > Thanks a lot!
> > 
> > Thanks,
> > Gao Xiang
> > 
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] erofs: derive atime instead of leaving it empty
  2020-10-31 19:51 ` [PATCH] erofs: derive atime instead of leaving it empty Gao Xiang
  2020-11-03  2:50   ` Gao Xiang
@ 2020-11-04  0:57   ` Chao Yu
  1 sibling, 0 replies; 5+ messages in thread
From: Chao Yu @ 2020-11-04  0:57 UTC (permalink / raw)
  To: Gao Xiang, linux-erofs, Chao Yu; +Cc: LKML, Gao Xiang, nl6720, stable

On 2020/11/1 3:51, Gao Xiang wrote:
> From: Gao Xiang <hsiangkao@redhat.com>
> 
> EROFS has _only one_ ondisk timestamp (ctime is currently
> documented and recorded, we might also record mtime instead
> with a new compat feature if needed) for each extended inode
> since EROFS isn't mainly for archival purposes so no need to
> keep all timestamps on disk especially for Android scenarios
> due to security concerns. Also, romfs/cramfs don't have their
> own on-disk timestamp, and squashfs only records mtime instead.
> 
> Let's also derive access time from ondisk timestamp rather than
> leaving it empty, and if mtime/atime for each file are really
> needed for specific scenarios as well, we can also use xattrs
> to record them then.
> 
> Reported-by: nl6720 <nl6720@gmail.com>
> [ Gao Xiang: It'd be better to backport for user-friendly concern. ]
> Fixes: 431339ba9042 ("staging: erofs: add inode operations")
> Cc: stable <stable@vger.kernel.org> # 4.19+
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Looks good to me. :)

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-04  0:57 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20201031195102.21221-1-hsiangkao.ref@aol.com>
2020-10-31 19:51 ` [PATCH] erofs: derive atime instead of leaving it empty Gao Xiang
2020-11-03  2:50   ` Gao Xiang
2020-11-03 15:58     ` Chao Yu
2020-11-03 17:04       ` Gao Xiang
2020-11-04  0:57   ` Chao Yu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).