diff mbox series

[SMB3,client] update allocation size more accurately on write completion

Message ID CAH2r5mtsvNU--3EDFvAPSVuSnLpmbDr5A4YbaY=9rrndLyOpiA@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series [SMB3,client] update allocation size more accurately on write completion | expand

Commit Message

Steve French Feb. 22, 2024, 6:40 a.m. UTC
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

Comments

Shyam Prasad N Feb. 23, 2024, 1 p.m. UTC | #1
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.
Steve French Feb. 23, 2024, 1:41 p.m. UTC | #2
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)
diff mbox series

Patch

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