| Message ID | 20240121033248.125282-3-sprasad@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] cifs: handle servers that still advertise multichannel after disabling | expand |
On Sun, Jan 21, 2024 at 9:03 AM <nspmangalore@gmail.com> wrote: > > From: Shyam Prasad N <sprasad@microsoft.com> > > SMB2 CLOSE command with SMB2_CLOSE_FLAG_POSTQUERY_ATTRIB > flag set is already used by the code for SMB3+. > smb2_close_getattr is the function that uses this to > update the inode attributes. > > However, we were skipping the EndOfFile info that's returned > by the server. There is a small chance that the file size > may have been changed in the small window between the client > sending the close request (thereby giving up lease if it had) > to the point that the server returns the response. > > This change uses the field to update the inode size. > Also, it is a valid case for a zero AllocationSize to be returned > by the server for the file. We were discarding such values, thereby > resulting in stale i_blocks value. Fixed that here too. > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > --- > fs/smb/client/smb2ops.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c > index d9553c2556a2..e23577584ed6 100644 > --- a/fs/smb/client/smb2ops.c > +++ b/fs/smb/client/smb2ops.c > @@ -1433,9 +1433,9 @@ smb2_close_getattr(const unsigned int xid, struct cifs_tcon *tcon, > * but instead 512 byte (2**9) size is required for > * calculating num blocks. > */ > - if (le64_to_cpu(file_inf.AllocationSize) > 4096) > - inode->i_blocks = > - (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9; > + inode->i_blocks = (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9; > + > + inode->i_size = le64_to_cpu(file_inf.EndOfFile); > > /* End of file and Attributes should not have to be updated on close */ > spin_unlock(&inode->i_lock); > -- > 2.34.1 > Noticed a couple of things in other places in the code: 1. i_size_write() should be called instead of updating i_size directly. 2. There's a server_eof field that is generally maintained in sync with i_size. Need to check how that needs to be done here. I'll submit a v2 of this patch soon.
On Tue, Jan 23, 2024 at 12:52 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Sun, Jan 21, 2024 at 9:03 AM <nspmangalore@gmail.com> wrote: > > > > From: Shyam Prasad N <sprasad@microsoft.com> > > > > SMB2 CLOSE command with SMB2_CLOSE_FLAG_POSTQUERY_ATTRIB > > flag set is already used by the code for SMB3+. > > smb2_close_getattr is the function that uses this to > > update the inode attributes. > > > > However, we were skipping the EndOfFile info that's returned > > by the server. There is a small chance that the file size > > may have been changed in the small window between the client > > sending the close request (thereby giving up lease if it had) > > to the point that the server returns the response. > > > > This change uses the field to update the inode size. > > Also, it is a valid case for a zero AllocationSize to be returned > > by the server for the file. We were discarding such values, thereby > > resulting in stale i_blocks value. Fixed that here too. > > > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > > --- > > fs/smb/client/smb2ops.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c > > index d9553c2556a2..e23577584ed6 100644 > > --- a/fs/smb/client/smb2ops.c > > +++ b/fs/smb/client/smb2ops.c > > @@ -1433,9 +1433,9 @@ smb2_close_getattr(const unsigned int xid, struct cifs_tcon *tcon, > > * but instead 512 byte (2**9) size is required for > > * calculating num blocks. > > */ > > - if (le64_to_cpu(file_inf.AllocationSize) > 4096) > > - inode->i_blocks = > > - (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9; > > + inode->i_blocks = (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9; > > + > > + inode->i_size = le64_to_cpu(file_inf.EndOfFile); > > > > /* End of file and Attributes should not have to be updated on close */ > > spin_unlock(&inode->i_lock); > > -- > > 2.34.1 > > > Noticed a couple of things in other places in the code: > 1. i_size_write() should be called instead of updating i_size directly. > 2. There's a server_eof field that is generally maintained in sync > with i_size. Need to check how that needs to be done here. > > I'll submit a v2 of this patch soon. > > -- > Regards, > Shyam Attached updated patch.
This was the patch that was causing test generic/074 to regress so I took it out of for-next temporarily till we can fix it. The version of the patch I was using is attached On Tue, Jan 23, 2024 at 1:48 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Tue, Jan 23, 2024 at 12:52 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > On Sun, Jan 21, 2024 at 9:03 AM <nspmangalore@gmail.com> wrote: > > > > > > From: Shyam Prasad N <sprasad@microsoft.com> > > > > > > SMB2 CLOSE command with SMB2_CLOSE_FLAG_POSTQUERY_ATTRIB > > > flag set is already used by the code for SMB3+. > > > smb2_close_getattr is the function that uses this to > > > update the inode attributes. > > > > > > However, we were skipping the EndOfFile info that's returned > > > by the server. There is a small chance that the file size > > > may have been changed in the small window between the client > > > sending the close request (thereby giving up lease if it had) > > > to the point that the server returns the response. > > > > > > This change uses the field to update the inode size. > > > Also, it is a valid case for a zero AllocationSize to be returned > > > by the server for the file. We were discarding such values, thereby > > > resulting in stale i_blocks value. Fixed that here too. > > > > > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > > > --- > > > fs/smb/client/smb2ops.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c > > > index d9553c2556a2..e23577584ed6 100644 > > > --- a/fs/smb/client/smb2ops.c > > > +++ b/fs/smb/client/smb2ops.c > > > @@ -1433,9 +1433,9 @@ smb2_close_getattr(const unsigned int xid, struct cifs_tcon *tcon, > > > * but instead 512 byte (2**9) size is required for > > > * calculating num blocks. > > > */ > > > - if (le64_to_cpu(file_inf.AllocationSize) > 4096) > > > - inode->i_blocks = > > > - (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9; > > > + inode->i_blocks = (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9; > > > + > > > + inode->i_size = le64_to_cpu(file_inf.EndOfFile); > > > > > > /* End of file and Attributes should not have to be updated on close */ > > > spin_unlock(&inode->i_lock); > > > -- > > > 2.34.1 > > > > > Noticed a couple of things in other places in the code: > > 1. i_size_write() should be called instead of updating i_size directly. > > 2. There's a server_eof field that is generally maintained in sync > > with i_size. Need to check how that needs to be done here. > > > > I'll submit a v2 of this patch soon. > > > > -- > > Regards, > > Shyam > > Attached updated patch. > > -- > Regards, > Shyam
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c index d9553c2556a2..e23577584ed6 100644 --- a/fs/smb/client/smb2ops.c +++ b/fs/smb/client/smb2ops.c @@ -1433,9 +1433,9 @@ smb2_close_getattr(const unsigned int xid, struct cifs_tcon *tcon, * but instead 512 byte (2**9) size is required for * calculating num blocks. */ - if (le64_to_cpu(file_inf.AllocationSize) > 4096) - inode->i_blocks = - (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9; + inode->i_blocks = (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9; + + inode->i_size = le64_to_cpu(file_inf.EndOfFile); /* End of file and Attributes should not have to be updated on close */ spin_unlock(&inode->i_lock);