diff mbox series

[v2,8/8] cifs: update the ctime on a partial page write

Message ID 20230612104524.17058-9-jlayton@kernel.org (mailing list archive)
State New, archived
Headers show
Series fs: add some missing ctime updates | expand

Commit Message

Jeff Layton June 12, 2023, 10:45 a.m. UTC
POSIX says:

    "Upon successful completion, where nbyte is greater than 0, write()
     shall mark for update the last data modification and last file status
     change timestamps of the file..."

Add the missing ctime update.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/smb/client/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Tom Talpey June 12, 2023, 1:41 p.m. UTC | #1
On 6/12/2023 6:45 AM, Jeff Layton wrote:
> POSIX says:
> 
>      "Upon successful completion, where nbyte is greater than 0, write()
>       shall mark for update the last data modification and last file status
>       change timestamps of the file..."
> 
> Add the missing ctime update.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/smb/client/file.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> index df88b8c04d03..a00038a326cf 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -2596,7 +2596,7 @@ static int cifs_partialpagewrite(struct page *page, unsigned from, unsigned to)
>   					   write_data, to - from, &offset);
>   		cifsFileInfo_put(open_file);
>   		/* Does mm or vfs already set times? */
> -		inode->i_atime = inode->i_mtime = current_time(inode);
> +		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);

Question. It appears that roughly half the filesystems in this series
don't touch the i_atime in this case. And the other half do. Which is
correct? Did they incorrectly set i_atime instead of i_ctime?

Tom.

>   		if ((bytes_written > 0) && (offset))
>   			rc = 0;
>   		else if (bytes_written < 0)
Jeff Layton June 12, 2023, 1:51 p.m. UTC | #2
On Mon, 2023-06-12 at 09:41 -0400, Tom Talpey wrote:
> On 6/12/2023 6:45 AM, Jeff Layton wrote:
> > POSIX says:
> > 
> >      "Upon successful completion, where nbyte is greater than 0, write()
> >       shall mark for update the last data modification and last file status
> >       change timestamps of the file..."
> > 
> > Add the missing ctime update.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >   fs/smb/client/file.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> > index df88b8c04d03..a00038a326cf 100644
> > --- a/fs/smb/client/file.c
> > +++ b/fs/smb/client/file.c
> > @@ -2596,7 +2596,7 @@ static int cifs_partialpagewrite(struct page *page, unsigned from, unsigned to)
> >   					   write_data, to - from, &offset);
> >   		cifsFileInfo_put(open_file);
> >   		/* Does mm or vfs already set times? */
> > -		inode->i_atime = inode->i_mtime = current_time(inode);
> > +		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
> 
> Question. It appears that roughly half the filesystems in this series
> don't touch the i_atime in this case. And the other half do. Which is
> correct? Did they incorrectly set i_atime instead of i_ctime?
> 

I noticed that too, and with this set, I decided to not make any atime
changes since I wasn't sure.

I think the answer to your question is "it depends". atime is supposed
to be updated on reads, not writes, but sometimes a write requires a RMW
cycle of some flavor so one can imagine that in some cases we'd need to
update all three.

In this case, I'm not sure that updating any of these times is the right
thing to do. This is called from ->launder_folio, so the syscall that
issued the write is long gone and we're in writeback here.

With NFS, we generally leave timestamp updates to the server. Should any
of these timestamps be updated by the (SMB1) client here?
diff mbox series

Patch

diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index df88b8c04d03..a00038a326cf 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -2596,7 +2596,7 @@  static int cifs_partialpagewrite(struct page *page, unsigned from, unsigned to)
 					   write_data, to - from, &offset);
 		cifsFileInfo_put(open_file);
 		/* Does mm or vfs already set times? */
-		inode->i_atime = inode->i_mtime = current_time(inode);
+		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
 		if ((bytes_written > 0) && (offset))
 			rc = 0;
 		else if (bytes_written < 0)