| Message ID | CAH2r5mtsvNU--3EDFvAPSVuSnLpmbDr5A4YbaY=9rrndLyOpiA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SMB3,client] update allocation size more accurately on write completion | expand |
On Thu, Feb 22, 2024 at 12:10 PM Steve French <smfrench@gmail.com> wrote: > > Changes to allocation size are approximated for extending writes of > cached files until the server returns the actual value (on SMB3 close > or query info for example), but it was setting the estimated value for > number of blocks to larger than the file size even if the file is > likely sparse which breaks various xfstests (e.g. generic/129, 130, > 221, 228). > > When i_size and i_blocks are updated in write completion do not > increase allocation size more than what was written (rounded up to 512 > bytes). > > See attached. > > This fixes the recent regression in various xfstests caused by the > xfstest change > > commit b4396efc75aba5325f22690303857af4f63d128e > Author: Alexander Patrakov <patrakov@gmail.com> > Date: Tue Dec 19 04:57:20 2023 +0800 > > _require_sparse_files: rewrite as a direct test instead of a black list > > > -- > Thanks, > > Steve + loff_t additional_blocks = (512 - 1 + copied) >> 9; There are chances that additional_blocks could lesser than this value. i.e. if the write started writes from before EOF. We could then calculate additional_blocks as more than it should be. I think there should be an additional check here: if (pos - copied) < i_size: additional_blocks should be based on pos - i_size. else additional_blocks should be based on copied. Regardless of this patch, is this really a bug? This is only an estimation that we do till we get the true value from the server. A filesystem is free to allocate blocks as necessary. This patch definitely improves this estimation though.
On Fri, Feb 23, 2024 at 7:00 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > Regardless of this patch, is this really a bug? This is only an > estimation that we do till we get the true value from the server. > A filesystem is free to allocate blocks as necessary. This patch > definitely improves this estimation though. It is needed to pass various xfstests (due to a recent change in late December of how xfstests checks), but I agree that this is just an improvement of an estimation (and in any case a local file system or a remote server filesystem can change allocation size for a file)
From ae33f1b691cc9fd6fc0dfe84981e5e8d5f0cd3d2 Mon Sep 17 00:00:00 2001 From: Steve French <stfrench@microsoft.com> Date: Thu, 22 Feb 2024 00:26:52 -0600 Subject: [PATCH] smb3: update allocation size more accurately on write completion Changes to allocation size are approximated for extending writes of cached files until the server returns the actual value (on SMB3 close or query info for example), but it was setting the estimated value for number of blocks to larger than the file size even if the file is likely sparse which breaks various xfstests (e.g. generic/129, 130, 221, 228). When i_size and i_blocks are updated in write completion do not increase allocation size more than what was written (rounded up to 512 bytes). Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/smb/client/file.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c index 05e915162f05..6b2da368d52d 100644 --- a/fs/smb/client/file.c +++ b/fs/smb/client/file.c @@ -3095,8 +3095,15 @@ static int cifs_write_end(struct file *file, struct address_space *mapping, if (rc > 0) { spin_lock(&inode->i_lock); if (pos > inode->i_size) { + loff_t additional_blocks = (512 - 1 + copied) >> 9; + i_size_write(inode, pos); - inode->i_blocks = (512 - 1 + pos) >> 9; + /* + * Estimate new allocation size based on the amount written. + * This will be updated from server on close (and on queryinfo) + */ + inode->i_blocks = min_t(blkcnt_t, (512 - 1 + pos) >> 9, + inode->i_blocks + additional_blocks); } spin_unlock(&inode->i_lock); } -- 2.40.1
Changes to allocation size are approximated for extending writes of cached files until the server returns the actual value (on SMB3 close or query info for example), but it was setting the estimated value for number of blocks to larger than the file size even if the file is likely sparse which breaks various xfstests (e.g. generic/129, 130, 221, 228). When i_size and i_blocks are updated in write completion do not increase allocation size more than what was written (rounded up to 512 bytes). See attached. This fixes the recent regression in various xfstests caused by the xfstest change commit b4396efc75aba5325f22690303857af4f63d128e Author: Alexander Patrakov <patrakov@gmail.com> Date: Tue Dec 19 04:57:20 2023 +0800 _require_sparse_files: rewrite as a direct test instead of a black list