* [PATCH 1/9] mm: Fix missing folio invalidation calls during truncation
2024-08-23 20:08 [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry David Howells
@ 2024-08-23 20:08 ` David Howells
2024-08-23 20:12 ` Matthew Wilcox
2024-08-28 11:38 ` Pankaj Raghav (Samsung)
2024-08-23 20:08 ` [PATCH 2/9] afs: Fix post-setattr file edit to do truncation correctly David Howells
` (9 subsequent siblings)
10 siblings, 2 replies; 14+ messages in thread
From: David Howells @ 2024-08-23 20:08 UTC (permalink / raw)
To: Christian Brauner, Steve French
Cc: David Howells, Pankaj Raghav, Paulo Alcantara, Jeff Layton,
Matthew Wilcox, netfs, linux-afs, linux-cifs, linux-nfs,
ceph-devel, v9fs, linux-erofs, linux-fsdevel, linux-mm,
linux-kernel, Marc Dionne
When AS_RELEASE_ALWAYS is set on a mapping, the ->release_folio() and
->invalidate_folio() calls should be invoked even if PG_private and
PG_private_2 aren't set. This is used by netfslib to keep track of the
point above which reads can be skipped in favour of just zeroing pagecache
locally.
There are a couple of places in truncation in which invalidation is only
called when folio_has_private() is true. Fix these to check
folio_needs_release() instead.
Without this, the generic/075 and generic/112 xfstests (both fsx-based
tests) fail with minimum folio size patches applied[1].
Fixes: b4fa966f03b7 ("mm, netfs, fscache: stop read optimisation when folio removed from pagecache")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Matthew Wilcox (Oracle) <willy@infradead.org>
cc: Pankaj Raghav <p.raghav@samsung.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
cc: netfs@lists.linux.dev
cc: linux-mm@kvack.org
cc: linux-fsdevel@vger.kernel.org
Link: https://lore.kernel.org/r/20240815090849.972355-1-kernel@pankajraghav.com/ [1]
---
mm/truncate.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/truncate.c b/mm/truncate.c
index 4d61fbdd4b2f..0668cd340a46 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -157,7 +157,7 @@ static void truncate_cleanup_folio(struct folio *folio)
if (folio_mapped(folio))
unmap_mapping_folio(folio);
- if (folio_has_private(folio))
+ if (folio_needs_release(folio))
folio_invalidate(folio, 0, folio_size(folio));
/*
@@ -219,7 +219,7 @@ bool truncate_inode_partial_folio(struct folio *folio, loff_t start, loff_t end)
if (!mapping_inaccessible(folio->mapping))
folio_zero_range(folio, offset, length);
- if (folio_has_private(folio))
+ if (folio_needs_release(folio))
folio_invalidate(folio, offset, length);
if (!folio_test_large(folio))
return true;
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 1/9] mm: Fix missing folio invalidation calls during truncation
2024-08-23 20:08 ` [PATCH 1/9] mm: Fix missing folio invalidation calls during truncation David Howells
@ 2024-08-23 20:12 ` Matthew Wilcox
2024-08-28 11:38 ` Pankaj Raghav (Samsung)
1 sibling, 0 replies; 14+ messages in thread
From: Matthew Wilcox @ 2024-08-23 20:12 UTC (permalink / raw)
To: David Howells
Cc: Christian Brauner, Steve French, Pankaj Raghav, Paulo Alcantara,
Jeff Layton, netfs, linux-afs, linux-cifs, linux-nfs, ceph-devel,
v9fs, linux-erofs, linux-fsdevel, linux-mm, linux-kernel,
Marc Dionne
On Fri, Aug 23, 2024 at 09:08:09PM +0100, David Howells wrote:
> When AS_RELEASE_ALWAYS is set on a mapping, the ->release_folio() and
> ->invalidate_folio() calls should be invoked even if PG_private and
> PG_private_2 aren't set. This is used by netfslib to keep track of the
> point above which reads can be skipped in favour of just zeroing pagecache
> locally.
>
> There are a couple of places in truncation in which invalidation is only
> called when folio_has_private() is true. Fix these to check
> folio_needs_release() instead.
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
I think we also want to change the folio_has_private() call in
mapping_evict_folio() to folio_test_private(). Same for the one
in migrate_vma_check_page().
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/9] mm: Fix missing folio invalidation calls during truncation
2024-08-23 20:08 ` [PATCH 1/9] mm: Fix missing folio invalidation calls during truncation David Howells
2024-08-23 20:12 ` Matthew Wilcox
@ 2024-08-28 11:38 ` Pankaj Raghav (Samsung)
1 sibling, 0 replies; 14+ messages in thread
From: Pankaj Raghav (Samsung) @ 2024-08-28 11:38 UTC (permalink / raw)
To: David Howells
Cc: Christian Brauner, Steve French, Pankaj Raghav, Paulo Alcantara,
Jeff Layton, Matthew Wilcox, netfs, linux-afs, linux-cifs,
linux-nfs, ceph-devel, v9fs, linux-erofs, linux-fsdevel, linux-mm,
linux-kernel, Marc Dionne
On Fri, Aug 23, 2024 at 09:08:09PM +0100, David Howells wrote:
> When AS_RELEASE_ALWAYS is set on a mapping, the ->release_folio() and
> ->invalidate_folio() calls should be invoked even if PG_private and
> PG_private_2 aren't set. This is used by netfslib to keep track of the
Should we update the comment in pagemap?
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 55b254d951da..18dd6174e6cc 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -204,7 +204,8 @@ enum mapping_flags {
AS_EXITING = 4, /* final truncate in progress */
/* writeback related tags are not used */
AS_NO_WRITEBACK_TAGS = 5,
- AS_RELEASE_ALWAYS = 6, /* Call ->release_folio(), even if no private data */
+ AS_RELEASE_ALWAYS = 6, /* Call ->release_folio() and ->invalidate_folio,
+ even if no private data */
AS_STABLE_WRITES = 7, /* must wait for writeback before modifying
folio contents */
AS_INACCESSIBLE = 8, /* Do not attempt direct R/W access to the mapping */
> point above which reads can be skipped in favour of just zeroing pagecache
> locally.
>
> There are a couple of places in truncation in which invalidation is only
> called when folio_has_private() is true. Fix these to check
> folio_needs_release() instead.
>
> Without this, the generic/075 and generic/112 xfstests (both fsx-based
> tests) fail with minimum folio size patches applied[1].
>
> Fixes: b4fa966f03b7 ("mm, netfs, fscache: stop read optimisation when folio removed from pagecache")
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> cc: Pankaj Raghav <p.raghav@samsung.com>
> cc: Jeff Layton <jlayton@kernel.org>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> cc: netfs@lists.linux.dev
> cc: linux-mm@kvack.org
> cc: linux-fsdevel@vger.kernel.org
> Link: https://lore.kernel.org/r/20240815090849.972355-1-kernel@pankajraghav.com/ [1]
> ---
> mm/truncate.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/truncate.c b/mm/truncate.c
> index 4d61fbdd4b2f..0668cd340a46 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -157,7 +157,7 @@ static void truncate_cleanup_folio(struct folio *folio)
> if (folio_mapped(folio))
> unmap_mapping_folio(folio);
>
> - if (folio_has_private(folio))
> + if (folio_needs_release(folio))
> folio_invalidate(folio, 0, folio_size(folio));
>
> /*
> @@ -219,7 +219,7 @@ bool truncate_inode_partial_folio(struct folio *folio, loff_t start, loff_t end)
> if (!mapping_inaccessible(folio->mapping))
> folio_zero_range(folio, offset, length);
>
> - if (folio_has_private(folio))
> + if (folio_needs_release(folio))
> folio_invalidate(folio, offset, length);
> if (!folio_test_large(folio))
> return true;
>
--
Pankaj Raghav
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/9] afs: Fix post-setattr file edit to do truncation correctly
2024-08-23 20:08 [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry David Howells
2024-08-23 20:08 ` [PATCH 1/9] mm: Fix missing folio invalidation calls during truncation David Howells
@ 2024-08-23 20:08 ` David Howells
2024-08-23 20:08 ` [PATCH 3/9] netfs: Fix netfs_release_folio() to say no if folio dirty David Howells
` (8 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: David Howells @ 2024-08-23 20:08 UTC (permalink / raw)
To: Christian Brauner, Steve French
Cc: David Howells, Pankaj Raghav, Paulo Alcantara, Jeff Layton,
Matthew Wilcox, netfs, linux-afs, linux-cifs, linux-nfs,
ceph-devel, v9fs, linux-erofs, linux-fsdevel, linux-mm,
linux-kernel, Marc Dionne
At the end of an kAFS RPC operation, there is an "edit" phase (originally
intended for post-directory modification ops to edit the local image) that
the setattr VFS op uses to fix up the pagecache if the RPC that requested
truncation of a file was successful.
afs_setattr_edit_file() calls truncate_setsize() which sets i_size, expands
the pagecache if needed and truncates the pagecache. The first two of
those, however, are redundant as they've already been done by
afs_setattr_success() under the io_lock and the first is also done under
the callback lock (cb_lock).
Fix afs_setattr_edit_file() to call truncate_pagecache() instead (which is
called by truncate_setsize(), thereby skipping the redundant parts.
Fixes: 100ccd18bb41 ("netfs: Optimise away reads above the point at which there can be no data")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Matthew Wilcox (Oracle) <willy@infradead.org>
cc: Pankaj Raghav <p.raghav@samsung.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
cc: netfs@lists.linux.dev
cc: linux-mm@kvack.org
cc: linux-fsdevel@vger.kernel.org
---
fs/afs/inode.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 3acf5e050072..a95e77670b49 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -695,13 +695,18 @@ static void afs_setattr_edit_file(struct afs_operation *op)
{
struct afs_vnode_param *vp = &op->file[0];
struct afs_vnode *vnode = vp->vnode;
+ struct inode *inode = &vnode->netfs.inode;
if (op->setattr.attr->ia_valid & ATTR_SIZE) {
loff_t size = op->setattr.attr->ia_size;
- loff_t i_size = op->setattr.old_i_size;
+ loff_t old = op->setattr.old_i_size;
+
+ /* Note: inode->i_size was updated by afs_apply_status() inside
+ * the I/O and callback locks.
+ */
- if (size != i_size) {
- truncate_setsize(&vnode->netfs.inode, size);
+ if (size != old) {
+ truncate_pagecache(inode, size);
netfs_resize_file(&vnode->netfs, size, true);
fscache_resize_cookie(afs_vnode_cache(vnode), size);
}
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 3/9] netfs: Fix netfs_release_folio() to say no if folio dirty
2024-08-23 20:08 [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry David Howells
2024-08-23 20:08 ` [PATCH 1/9] mm: Fix missing folio invalidation calls during truncation David Howells
2024-08-23 20:08 ` [PATCH 2/9] afs: Fix post-setattr file edit to do truncation correctly David Howells
@ 2024-08-23 20:08 ` David Howells
2024-08-23 20:08 ` [PATCH 4/9] netfs: Fix trimming of streaming-write folios in netfs_inval_folio() David Howells
` (7 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: David Howells @ 2024-08-23 20:08 UTC (permalink / raw)
To: Christian Brauner, Steve French
Cc: David Howells, Pankaj Raghav, Paulo Alcantara, Jeff Layton,
Matthew Wilcox, netfs, linux-afs, linux-cifs, linux-nfs,
ceph-devel, v9fs, linux-erofs, linux-fsdevel, linux-mm,
linux-kernel, Marc Dionne
Fix netfs_release_folio() to say no (ie. return false) if the folio is
dirty (analogous with iomap's behaviour). Without this, it will say yes to
the release of a dirty page by split_huge_page_to_list_to_order(), which
will result in the loss of untruncated data in the folio.
Without this, the generic/075 and generic/112 xfstests (both fsx-based
tests) fail with minimum folio size patches applied[1].
Fixes: c1ec4d7c2e13 ("netfs: Provide invalidate_folio and release_folio calls")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Matthew Wilcox (Oracle) <willy@infradead.org>
cc: Pankaj Raghav <p.raghav@samsung.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
cc: netfs@lists.linux.dev
cc: linux-mm@kvack.org
cc: linux-fsdevel@vger.kernel.org
Link: https://lore.kernel.org/r/20240815090849.972355-1-kernel@pankajraghav.com/ [1]
---
fs/netfs/misc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/netfs/misc.c b/fs/netfs/misc.c
index 554a1a4615ad..69324761fcf7 100644
--- a/fs/netfs/misc.c
+++ b/fs/netfs/misc.c
@@ -161,6 +161,9 @@ bool netfs_release_folio(struct folio *folio, gfp_t gfp)
struct netfs_inode *ctx = netfs_inode(folio_inode(folio));
unsigned long long end;
+ if (folio_test_dirty(folio))
+ return false;
+
end = folio_pos(folio) + folio_size(folio);
if (end > ctx->zero_point)
ctx->zero_point = end;
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 4/9] netfs: Fix trimming of streaming-write folios in netfs_inval_folio()
2024-08-23 20:08 [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry David Howells
` (2 preceding siblings ...)
2024-08-23 20:08 ` [PATCH 3/9] netfs: Fix netfs_release_folio() to say no if folio dirty David Howells
@ 2024-08-23 20:08 ` David Howells
2024-08-23 20:08 ` [PATCH 5/9] netfs: Fix missing iterator reset on retry of short read David Howells
` (6 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: David Howells @ 2024-08-23 20:08 UTC (permalink / raw)
To: Christian Brauner, Steve French
Cc: David Howells, Pankaj Raghav, Paulo Alcantara, Jeff Layton,
Matthew Wilcox, netfs, linux-afs, linux-cifs, linux-nfs,
ceph-devel, v9fs, linux-erofs, linux-fsdevel, linux-mm,
linux-kernel, Marc Dionne
When netfslib writes to a folio that it doesn't have data for, but that
data exists on the server, it will make a 'streaming write' whereby it
stores data in a folio that is marked dirty, but not uptodate. When it
does this, it attaches a record to folio->private to track the dirty
region.
When truncate() or fallocate() wants to invalidate part of such a folio, it
will call into ->invalidate_folio(), specifying the part of the folio that
is to be invalidated. netfs_invalidate_folio(), on behalf of the
filesystem, must then determine how to trim the streaming write record. In
a couple of cases, however, it does this incorrectly (the reduce-length and
move-start cases are switched over and don't, in any case, calculate the
value correctly).
Fix this by making the logic tree more obvious and fixing the cases.
Fixes: 9ebff83e6481 ("netfs: Prep to use folio->private for write grouping and streaming write")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Matthew Wilcox (Oracle) <willy@infradead.org>
cc: Pankaj Raghav <p.raghav@samsung.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
cc: netfs@lists.linux.dev
cc: linux-mm@kvack.org
cc: linux-fsdevel@vger.kernel.org
---
fs/netfs/misc.c | 50 ++++++++++++++++++++++++++++++++++---------------
1 file changed, 35 insertions(+), 15 deletions(-)
diff --git a/fs/netfs/misc.c b/fs/netfs/misc.c
index 69324761fcf7..c1f321cf5999 100644
--- a/fs/netfs/misc.c
+++ b/fs/netfs/misc.c
@@ -97,10 +97,20 @@ EXPORT_SYMBOL(netfs_clear_inode_writeback);
void netfs_invalidate_folio(struct folio *folio, size_t offset, size_t length)
{
struct netfs_folio *finfo;
+ struct netfs_inode *ctx = netfs_inode(folio_inode(folio));
size_t flen = folio_size(folio);
_enter("{%lx},%zx,%zx", folio->index, offset, length);
+ if (offset == 0 && length == flen) {
+ unsigned long long i_size = i_size_read(&ctx->inode);
+ unsigned long long fpos = folio_pos(folio), end;
+
+ end = umin(fpos + flen, i_size);
+ if (fpos < i_size && end > ctx->zero_point)
+ ctx->zero_point = end;
+ }
+
folio_wait_private_2(folio); /* [DEPRECATED] */
if (!folio_test_private(folio))
@@ -115,18 +125,34 @@ void netfs_invalidate_folio(struct folio *folio, size_t offset, size_t length)
/* We have a partially uptodate page from a streaming write. */
unsigned int fstart = finfo->dirty_offset;
unsigned int fend = fstart + finfo->dirty_len;
- unsigned int end = offset + length;
+ unsigned int iend = offset + length;
if (offset >= fend)
return;
- if (end <= fstart)
+ if (iend <= fstart)
+ return;
+
+ /* The invalidation region overlaps the data. If the region
+ * covers the start of the data, we either move along the start
+ * or just erase the data entirely.
+ */
+ if (offset <= fstart) {
+ if (iend >= fend)
+ goto erase_completely;
+ /* Move the start of the data. */
+ finfo->dirty_len = fend - iend;
+ finfo->dirty_offset = offset;
+ return;
+ }
+
+ /* Reduce the length of the data if the invalidation region
+ * covers the tail part.
+ */
+ if (iend >= fend) {
+ finfo->dirty_len = offset - fstart;
return;
- if (offset <= fstart && end >= fend)
- goto erase_completely;
- if (offset <= fstart && end > fstart)
- goto reduce_len;
- if (offset > fstart && end >= fend)
- goto move_start;
+ }
+
/* A partial write was split. The caller has already zeroed
* it, so just absorb the hole.
*/
@@ -139,12 +165,6 @@ void netfs_invalidate_folio(struct folio *folio, size_t offset, size_t length)
folio_clear_uptodate(folio);
kfree(finfo);
return;
-reduce_len:
- finfo->dirty_len = offset + length - finfo->dirty_offset;
- return;
-move_start:
- finfo->dirty_len -= offset - finfo->dirty_offset;
- finfo->dirty_offset = offset;
}
EXPORT_SYMBOL(netfs_invalidate_folio);
@@ -164,7 +184,7 @@ bool netfs_release_folio(struct folio *folio, gfp_t gfp)
if (folio_test_dirty(folio))
return false;
- end = folio_pos(folio) + folio_size(folio);
+ end = umin(folio_pos(folio) + folio_size(folio), i_size_read(&ctx->inode));
if (end > ctx->zero_point)
ctx->zero_point = end;
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 5/9] netfs: Fix missing iterator reset on retry of short read
2024-08-23 20:08 [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry David Howells
` (3 preceding siblings ...)
2024-08-23 20:08 ` [PATCH 4/9] netfs: Fix trimming of streaming-write folios in netfs_inval_folio() David Howells
@ 2024-08-23 20:08 ` David Howells
2024-08-23 20:08 ` [PATCH 6/9] cifs: Fix lack of credit renegotiation on read retry David Howells
` (5 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: David Howells @ 2024-08-23 20:08 UTC (permalink / raw)
To: Christian Brauner, Steve French
Cc: David Howells, Pankaj Raghav, Paulo Alcantara, Jeff Layton,
Matthew Wilcox, netfs, linux-afs, linux-cifs, linux-nfs,
ceph-devel, v9fs, linux-erofs, linux-fsdevel, linux-mm,
linux-kernel
Fix netfs_rreq_perform_resubmissions() to reset before retrying a short
read, otherwise the wrong part of the output buffer will be used.
Fixes: 92b6cc5d1e7c ("netfs: Add iov_iters to (sub)requests to describe various buffers")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Steve French <sfrench@samba.org>
cc: Paulo Alcantara <pc@manguebit.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cifs@vger.kernel.org
cc: netfs@lists.linux.dev
cc: linux-fsdevel@vger.kernel.org
---
fs/netfs/io.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/netfs/io.c b/fs/netfs/io.c
index 5367caf3fa28..4da0a494e860 100644
--- a/fs/netfs/io.c
+++ b/fs/netfs/io.c
@@ -313,6 +313,7 @@ static bool netfs_rreq_perform_resubmissions(struct netfs_io_request *rreq)
netfs_reset_subreq_iter(rreq, subreq);
netfs_read_from_server(rreq, subreq);
} else if (test_bit(NETFS_SREQ_SHORT_IO, &subreq->flags)) {
+ netfs_reset_subreq_iter(rreq, subreq);
netfs_rreq_short_read(rreq, subreq);
}
}
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 6/9] cifs: Fix lack of credit renegotiation on read retry
2024-08-23 20:08 [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry David Howells
` (4 preceding siblings ...)
2024-08-23 20:08 ` [PATCH 5/9] netfs: Fix missing iterator reset on retry of short read David Howells
@ 2024-08-23 20:08 ` David Howells
2024-08-23 20:08 ` [PATCH 7/9] netfs, cifs: Fix handling of short DIO read David Howells
` (4 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: David Howells @ 2024-08-23 20:08 UTC (permalink / raw)
To: Christian Brauner, Steve French
Cc: David Howells, Pankaj Raghav, Paulo Alcantara, Jeff Layton,
Matthew Wilcox, netfs, linux-afs, linux-cifs, linux-nfs,
ceph-devel, v9fs, linux-erofs, linux-fsdevel, linux-mm,
linux-kernel, Christian Brauner
When netfslib asks cifs to issue a read operation, it prefaces this with a
call to ->clamp_length() which cifs uses to negotiate credits, providing
receive capacity on the server; however, in the event that a read op needs
reissuing, netfslib doesn't call ->clamp_length() again as that could
shorten the subrequest, leaving a gap.
This causes the retried read to be done with zero credits which causes the
server to reject it with STATUS_INVALID_PARAMETER. This is a problem for a
DIO read that is requested that would go over the EOF. The short read will
be retried, causing EINVAL to be returned to the user when it fails.
Fix this by making cifs_req_issue_read() negotiate new credits if retrying
(NETFS_SREQ_RETRYING now gets set in the read side as well as the write
side in this instance).
This isn't sufficient, however: the new credits might not be sufficient to
complete the remainder of the read, so also add an additional field,
rreq->actual_len, that holds the actual size of the op we want to perform
without having to alter subreq->len.
We then rely on repeated short reads being retried until we finish the read
or reach the end of file and make a zero-length read.
Also fix a couple of places where the subrequest start and length need to
be altered by the amount so far transferred when being used.
Fixes: 69c3c023af25 ("cifs: Implement netfslib hooks")
Signed-off-by: David Howells <dhowells@redhat.com>
Link: https://lore.kernel.org/r/20240822220650.318774-2-dhowells@redhat.com
cc: Steve French <sfrench@samba.org>
cc: Paulo Alcantara <pc@manguebit.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cifs@vger.kernel.org
cc: netfs@lists.linux.dev
cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
fs/netfs/io.c | 2 ++
fs/smb/client/cifsglob.h | 1 +
fs/smb/client/file.c | 37 +++++++++++++++++++++++++++++++++----
fs/smb/client/smb2ops.c | 2 +-
fs/smb/client/smb2pdu.c | 28 +++++++++++++++++-----------
fs/smb/client/trace.h | 1 +
6 files changed, 55 insertions(+), 16 deletions(-)
diff --git a/fs/netfs/io.c b/fs/netfs/io.c
index 4da0a494e860..3303b515b536 100644
--- a/fs/netfs/io.c
+++ b/fs/netfs/io.c
@@ -306,6 +306,7 @@ static bool netfs_rreq_perform_resubmissions(struct netfs_io_request *rreq)
break;
subreq->source = NETFS_DOWNLOAD_FROM_SERVER;
subreq->error = 0;
+ __set_bit(NETFS_SREQ_RETRYING, &subreq->flags);
netfs_stat(&netfs_n_rh_download_instead);
trace_netfs_sreq(subreq, netfs_sreq_trace_download_instead);
netfs_get_subrequest(subreq, netfs_sreq_trace_get_resubmit);
@@ -313,6 +314,7 @@ static bool netfs_rreq_perform_resubmissions(struct netfs_io_request *rreq)
netfs_reset_subreq_iter(rreq, subreq);
netfs_read_from_server(rreq, subreq);
} else if (test_bit(NETFS_SREQ_SHORT_IO, &subreq->flags)) {
+ __set_bit(NETFS_SREQ_RETRYING, &subreq->flags);
netfs_reset_subreq_iter(rreq, subreq);
netfs_rreq_short_read(rreq, subreq);
}
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 5c9b3e6cd95f..ffd8373bb4b1 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1486,6 +1486,7 @@ struct cifs_io_subrequest {
struct cifs_io_request *req;
};
ssize_t got_bytes;
+ size_t actual_len;
unsigned int xid;
int result;
bool have_xid;
diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index 1fc66bcf49eb..b94802438c62 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -111,6 +111,7 @@ static void cifs_issue_write(struct netfs_io_subrequest *subreq)
goto fail;
}
+ wdata->actual_len = wdata->subreq.len;
rc = adjust_credits(wdata->server, wdata, cifs_trace_rw_credits_issue_write_adjust);
if (rc)
goto fail;
@@ -153,7 +154,7 @@ static bool cifs_clamp_length(struct netfs_io_subrequest *subreq)
struct cifs_io_request *req = container_of(subreq->rreq, struct cifs_io_request, rreq);
struct TCP_Server_Info *server = req->server;
struct cifs_sb_info *cifs_sb = CIFS_SB(rreq->inode->i_sb);
- size_t rsize = 0;
+ size_t rsize;
int rc;
rdata->xid = get_xid();
@@ -166,8 +167,8 @@ static bool cifs_clamp_length(struct netfs_io_subrequest *subreq)
cifs_sb->ctx);
- rc = server->ops->wait_mtu_credits(server, cifs_sb->ctx->rsize, &rsize,
- &rdata->credits);
+ rc = server->ops->wait_mtu_credits(server, cifs_sb->ctx->rsize,
+ &rsize, &rdata->credits);
if (rc) {
subreq->error = rc;
return false;
@@ -183,7 +184,8 @@ static bool cifs_clamp_length(struct netfs_io_subrequest *subreq)
server->credits, server->in_flight, 0,
cifs_trace_rw_credits_read_submit);
- subreq->len = min_t(size_t, subreq->len, rsize);
+ subreq->len = umin(subreq->len, rsize);
+ rdata->actual_len = subreq->len;
#ifdef CONFIG_CIFS_SMB_DIRECT
if (server->smbd_conn)
@@ -203,12 +205,39 @@ static void cifs_req_issue_read(struct netfs_io_subrequest *subreq)
struct netfs_io_request *rreq = subreq->rreq;
struct cifs_io_subrequest *rdata = container_of(subreq, struct cifs_io_subrequest, subreq);
struct cifs_io_request *req = container_of(subreq->rreq, struct cifs_io_request, rreq);
+ struct TCP_Server_Info *server = req->server;
+ struct cifs_sb_info *cifs_sb = CIFS_SB(rreq->inode->i_sb);
int rc = 0;
cifs_dbg(FYI, "%s: op=%08x[%x] mapping=%p len=%zu/%zu\n",
__func__, rreq->debug_id, subreq->debug_index, rreq->mapping,
subreq->transferred, subreq->len);
+ if (test_bit(NETFS_SREQ_RETRYING, &subreq->flags)) {
+ /*
+ * As we're issuing a retry, we need to negotiate some new
+ * credits otherwise the server may reject the op with
+ * INVALID_PARAMETER. Note, however, we may get back less
+ * credit than we need to complete the op, in which case, we
+ * shorten the op and rely on additional rounds of retry.
+ */
+ size_t rsize = umin(subreq->len - subreq->transferred,
+ cifs_sb->ctx->rsize);
+
+ rc = server->ops->wait_mtu_credits(server, rsize, &rdata->actual_len,
+ &rdata->credits);
+ if (rc)
+ goto out;
+
+ rdata->credits.in_flight_check = 1;
+
+ trace_smb3_rw_credits(rdata->rreq->debug_id,
+ rdata->subreq.debug_index,
+ rdata->credits.value,
+ server->credits, server->in_flight, 0,
+ cifs_trace_rw_credits_read_resubmit);
+ }
+
if (req->cfile->invalidHandle) {
do {
rc = cifs_reopen_file(req->cfile, true);
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 322cabc69c6f..14c3d11869ba 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -301,7 +301,7 @@ smb2_adjust_credits(struct TCP_Server_Info *server,
unsigned int /*enum smb3_rw_credits_trace*/ trace)
{
struct cifs_credits *credits = &subreq->credits;
- int new_val = DIV_ROUND_UP(subreq->subreq.len, SMB2_MAX_BUFFER_SIZE);
+ int new_val = DIV_ROUND_UP(subreq->actual_len, SMB2_MAX_BUFFER_SIZE);
int scredits, in_flight;
if (!credits->value || credits->value == new_val)
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 83facb54276a..d80107d1ba9e 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -4530,9 +4530,9 @@ smb2_readv_callback(struct mid_q_entry *mid)
"rdata server %p != mid server %p",
rdata->server, mid->server);
- cifs_dbg(FYI, "%s: mid=%llu state=%d result=%d bytes=%zu\n",
+ cifs_dbg(FYI, "%s: mid=%llu state=%d result=%d bytes=%zu/%zu\n",
__func__, mid->mid, mid->mid_state, rdata->result,
- rdata->subreq.len);
+ rdata->actual_len, rdata->subreq.len - rdata->subreq.transferred);
switch (mid->mid_state) {
case MID_RESPONSE_RECEIVED:
@@ -4586,15 +4586,18 @@ smb2_readv_callback(struct mid_q_entry *mid)
rdata->subreq.debug_index,
rdata->xid,
rdata->req->cfile->fid.persistent_fid,
- tcon->tid, tcon->ses->Suid, rdata->subreq.start,
- rdata->subreq.len, rdata->result);
+ tcon->tid, tcon->ses->Suid,
+ rdata->subreq.start + rdata->subreq.transferred,
+ rdata->actual_len,
+ rdata->result);
} else
trace_smb3_read_done(rdata->rreq->debug_id,
rdata->subreq.debug_index,
rdata->xid,
rdata->req->cfile->fid.persistent_fid,
tcon->tid, tcon->ses->Suid,
- rdata->subreq.start, rdata->got_bytes);
+ rdata->subreq.start + rdata->subreq.transferred,
+ rdata->got_bytes);
if (rdata->result == -ENODATA) {
/* We may have got an EOF error because fallocate
@@ -4622,6 +4625,7 @@ smb2_async_readv(struct cifs_io_subrequest *rdata)
{
int rc, flags = 0;
char *buf;
+ struct netfs_io_subrequest *subreq = &rdata->subreq;
struct smb2_hdr *shdr;
struct cifs_io_parms io_parms;
struct smb_rqst rqst = { .rq_iov = rdata->iov,
@@ -4632,15 +4636,15 @@ smb2_async_readv(struct cifs_io_subrequest *rdata)
int credit_request;
cifs_dbg(FYI, "%s: offset=%llu bytes=%zu\n",
- __func__, rdata->subreq.start, rdata->subreq.len);
+ __func__, subreq->start, subreq->len);
if (!rdata->server)
rdata->server = cifs_pick_channel(tcon->ses);
io_parms.tcon = tlink_tcon(rdata->req->cfile->tlink);
io_parms.server = server = rdata->server;
- io_parms.offset = rdata->subreq.start;
- io_parms.length = rdata->subreq.len;
+ io_parms.offset = subreq->start + subreq->transferred;
+ io_parms.length = rdata->actual_len;
io_parms.persistent_fid = rdata->req->cfile->fid.persistent_fid;
io_parms.volatile_fid = rdata->req->cfile->fid.volatile_fid;
io_parms.pid = rdata->req->pid;
@@ -4655,11 +4659,13 @@ smb2_async_readv(struct cifs_io_subrequest *rdata)
rdata->iov[0].iov_base = buf;
rdata->iov[0].iov_len = total_len;
+ rdata->got_bytes = 0;
+ rdata->result = 0;
shdr = (struct smb2_hdr *)buf;
if (rdata->credits.value > 0) {
- shdr->CreditCharge = cpu_to_le16(DIV_ROUND_UP(rdata->subreq.len,
+ shdr->CreditCharge = cpu_to_le16(DIV_ROUND_UP(rdata->actual_len,
SMB2_MAX_BUFFER_SIZE));
credit_request = le16_to_cpu(shdr->CreditCharge) + 8;
if (server->credits >= server->max_credits)
@@ -4683,11 +4689,11 @@ smb2_async_readv(struct cifs_io_subrequest *rdata)
if (rc) {
cifs_stats_fail_inc(io_parms.tcon, SMB2_READ_HE);
trace_smb3_read_err(rdata->rreq->debug_id,
- rdata->subreq.debug_index,
+ subreq->debug_index,
rdata->xid, io_parms.persistent_fid,
io_parms.tcon->tid,
io_parms.tcon->ses->Suid,
- io_parms.offset, io_parms.length, rc);
+ io_parms.offset, rdata->actual_len, rc);
}
async_readv_out:
diff --git a/fs/smb/client/trace.h b/fs/smb/client/trace.h
index 0f0c10c7ada7..8e9964001e2a 100644
--- a/fs/smb/client/trace.h
+++ b/fs/smb/client/trace.h
@@ -30,6 +30,7 @@
EM(cifs_trace_rw_credits_old_session, "old-session") \
EM(cifs_trace_rw_credits_read_response_add, "rd-resp-add") \
EM(cifs_trace_rw_credits_read_response_clear, "rd-resp-clr") \
+ EM(cifs_trace_rw_credits_read_resubmit, "rd-resubmit") \
EM(cifs_trace_rw_credits_read_submit, "rd-submit ") \
EM(cifs_trace_rw_credits_write_prepare, "wr-prepare ") \
EM(cifs_trace_rw_credits_write_response_add, "wr-resp-add") \
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 7/9] netfs, cifs: Fix handling of short DIO read
2024-08-23 20:08 [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry David Howells
` (5 preceding siblings ...)
2024-08-23 20:08 ` [PATCH 6/9] cifs: Fix lack of credit renegotiation on read retry David Howells
@ 2024-08-23 20:08 ` David Howells
2024-08-23 20:08 ` [PATCH 8/9] cifs: Fix FALLOC_FL_PUNCH_HOLE support David Howells
` (3 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: David Howells @ 2024-08-23 20:08 UTC (permalink / raw)
To: Christian Brauner, Steve French
Cc: David Howells, Pankaj Raghav, Paulo Alcantara, Jeff Layton,
Matthew Wilcox, netfs, linux-afs, linux-cifs, linux-nfs,
ceph-devel, v9fs, linux-erofs, linux-fsdevel, linux-mm,
linux-kernel, Christian Brauner
Short DIO reads, particularly in relation to cifs, are not being handled
correctly by cifs and netfslib. This can be tested by doing a DIO read of
a file where the size of read is larger than the size of the file. When it
crosses the EOF, it gets a short read and this gets retried, and in the
case of cifs, the retry read fails, with the failure being translated to
ENODATA.
Fix this by the following means:
(1) Add a flag, NETFS_SREQ_HIT_EOF, for the filesystem to set when it
detects that the read did hit the EOF.
(2) Make the netfslib read assessment stop processing subrequests when it
encounters one with that flag set.
(3) Return rreq->transferred, the accumulated contiguous amount read to
that point, to userspace for a DIO read.
(4) Make cifs set the flag and clear the error if the read RPC returned
ENODATA.
(5) Make cifs set the flag and clear the error if a short read occurred
without error and the read-to file position is now at the remote inode
size.
Fixes: 69c3c023af25 ("cifs: Implement netfslib hooks")
Signed-off-by: David Howells <dhowells@redhat.com>
Link: https://lore.kernel.org/r/20240822220650.318774-3-dhowells@redhat.com
cc: Steve French <sfrench@samba.org>
cc: Paulo Alcantara <pc@manguebit.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cifs@vger.kernel.org
cc: netfs@lists.linux.dev
cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
fs/netfs/io.c | 17 +++++++++++------
fs/smb/client/smb2pdu.c | 13 +++++++++----
include/linux/netfs.h | 1 +
3 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/fs/netfs/io.c b/fs/netfs/io.c
index 3303b515b536..943128507af5 100644
--- a/fs/netfs/io.c
+++ b/fs/netfs/io.c
@@ -368,7 +368,8 @@ static void netfs_rreq_assess_dio(struct netfs_io_request *rreq)
if (subreq->error || subreq->transferred == 0)
break;
transferred += subreq->transferred;
- if (subreq->transferred < subreq->len)
+ if (subreq->transferred < subreq->len ||
+ test_bit(NETFS_SREQ_HIT_EOF, &subreq->flags))
break;
}
@@ -503,7 +504,8 @@ void netfs_subreq_terminated(struct netfs_io_subrequest *subreq,
subreq->error = 0;
subreq->transferred += transferred_or_error;
- if (subreq->transferred < subreq->len)
+ if (subreq->transferred < subreq->len &&
+ !test_bit(NETFS_SREQ_HIT_EOF, &subreq->flags))
goto incomplete;
complete:
@@ -782,10 +784,13 @@ int netfs_begin_read(struct netfs_io_request *rreq, bool sync)
TASK_UNINTERRUPTIBLE);
ret = rreq->error;
- if (ret == 0 && rreq->submitted < rreq->len &&
- rreq->origin != NETFS_DIO_READ) {
- trace_netfs_failure(rreq, NULL, ret, netfs_fail_short_read);
- ret = -EIO;
+ if (ret == 0) {
+ if (rreq->origin == NETFS_DIO_READ) {
+ ret = rreq->transferred;
+ } else if (rreq->submitted < rreq->len) {
+ trace_netfs_failure(rreq, NULL, ret, netfs_fail_short_read);
+ ret = -EIO;
+ }
}
} else {
/* If we decrement nr_outstanding to 0, the ref belongs to us. */
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index d80107d1ba9e..9829784e8ec5 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -4507,6 +4507,7 @@ static void
smb2_readv_callback(struct mid_q_entry *mid)
{
struct cifs_io_subrequest *rdata = mid->callback_data;
+ struct netfs_inode *ictx = netfs_inode(rdata->rreq->inode);
struct cifs_tcon *tcon = tlink_tcon(rdata->req->cfile->tlink);
struct TCP_Server_Info *server = rdata->server;
struct smb2_hdr *shdr =
@@ -4600,11 +4601,15 @@ smb2_readv_callback(struct mid_q_entry *mid)
rdata->got_bytes);
if (rdata->result == -ENODATA) {
- /* We may have got an EOF error because fallocate
- * failed to enlarge the file.
- */
- if (rdata->subreq.start < rdata->subreq.rreq->i_size)
+ __set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
+ rdata->result = 0;
+ } else {
+ if (rdata->got_bytes < rdata->actual_len &&
+ rdata->subreq.start + rdata->subreq.transferred + rdata->got_bytes ==
+ ictx->remote_i_size) {
+ __set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
rdata->result = 0;
+ }
}
trace_smb3_rw_credits(rreq_debug_id, subreq_debug_index, rdata->credits.value,
server->credits, server->in_flight,
diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index 983816608f15..c47443e7a97e 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -198,6 +198,7 @@ struct netfs_io_subrequest {
#define NETFS_SREQ_NEED_RETRY 9 /* Set if the filesystem requests a retry */
#define NETFS_SREQ_RETRYING 10 /* Set if we're retrying */
#define NETFS_SREQ_FAILED 11 /* Set if the subreq failed unretryably */
+#define NETFS_SREQ_HIT_EOF 12 /* Set if we hit the EOF */
};
enum netfs_io_origin {
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 8/9] cifs: Fix FALLOC_FL_PUNCH_HOLE support
2024-08-23 20:08 [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry David Howells
` (6 preceding siblings ...)
2024-08-23 20:08 ` [PATCH 7/9] netfs, cifs: Fix handling of short DIO read David Howells
@ 2024-08-23 20:08 ` David Howells
2024-08-23 20:08 ` [PATCH 9/9] netfs, cifs: Improve some debugging bits David Howells
` (2 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: David Howells @ 2024-08-23 20:08 UTC (permalink / raw)
To: Christian Brauner, Steve French
Cc: David Howells, Pankaj Raghav, Paulo Alcantara, Jeff Layton,
Matthew Wilcox, netfs, linux-afs, linux-cifs, linux-nfs,
ceph-devel, v9fs, linux-erofs, linux-fsdevel, linux-mm,
linux-kernel, Shyam Prasad N
The cifs filesystem doesn't quite emulate FALLOC_FL_PUNCH_HOLE correctly
(note that due to lack of protocol support, it can't actually implement it
directly). Whilst it will (partially) invalidate dirty folios in the
pagecache, it doesn't write them back first, and so the EOF marker on the
server may be lower than inode->i_size.
This presents a problem, however, as if the punched hole invalidates the
tail of the locally cached dirty data, writeback won't know it needs to
move the EOF over to account for the hole punch (which isn't supposed to
move the EOF). We could just write zeroes over the punched out region of
the pagecache and write that back - but this is supposed to be a
deallocatory operation.
Fix this by manually moving the EOF over on the server after the operation
if the hole punched would corrupt it.
Note that the FSCTL_SET_ZERO_DATA RPC and the setting of the EOF should
probably be compounded to stop a third party interfering (or, at least,
massively reduce the chance).
This was reproducible occasionally by using fsx with the following script:
truncate 0x0 0x375e2 0x0
punch_hole 0x2f6d3 0x6ab5 0x375e2
truncate 0x0 0x3a71f 0x375e2
mapread 0xee05 0xcf12 0x3a71f
write 0x2078e 0x5604 0x3a71f
write 0x3ebdf 0x1421 0x3a71f *
punch_hole 0x379d0 0x8630 0x40000 *
mapread 0x2aaa2 0x85b 0x40000
fallocate 0x1b401 0x9ada 0x40000
read 0x15f2 0x7d32 0x40000
read 0x32f37 0x7a3b 0x40000 *
The second "write" should extend the EOF to 0x40000, and the "punch_hole"
should operate inside of that - but that depends on whether the VM gets in
and writes back the data first. If it doesn't, the file ends up 0x3a71f in
size, not 0x40000.
Fixes: 31742c5a3317 ("enable fallocate punch hole ("fallocate -p") for SMB3")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Steve French <sfrench@samba.org>
cc: Paulo Alcantara <pc@manguebit.com>
cc: Shyam Prasad N <nspmangalore@gmail.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cifs@vger.kernel.org
cc: netfs@lists.linux.dev
---
fs/smb/client/smb2ops.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 14c3d11869ba..8fb68c47c218 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -3305,6 +3305,7 @@ static long smb3_punch_hole(struct file *file, struct cifs_tcon *tcon,
struct inode *inode = file_inode(file);
struct cifsFileInfo *cfile = file->private_data;
struct file_zero_data_information fsctl_buf;
+ unsigned long long end = offset + len, i_size, remote_i_size;
long rc;
unsigned int xid;
__u8 set_sparse = 1;
@@ -3336,6 +3337,27 @@ static long smb3_punch_hole(struct file *file, struct cifs_tcon *tcon,
(char *)&fsctl_buf,
sizeof(struct file_zero_data_information),
CIFSMaxBufSize, NULL, NULL);
+
+ if (rc)
+ goto unlock;
+
+ /* If there's dirty data in the buffer that would extend the EOF if it
+ * were written, then we need to move the EOF marker over to the lower
+ * of the high end of the hole and the proposed EOF. The problem is
+ * that we locally hole-punch the tail of the dirty data, the proposed
+ * EOF update will end up in the wrong place.
+ */
+ i_size = i_size_read(inode);
+ remote_i_size = netfs_inode(inode)->remote_i_size;
+ if (end > remote_i_size && i_size > remote_i_size) {
+ unsigned long long extend_to = umin(end, i_size);
+ rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
+ cfile->fid.volatile_fid, cfile->pid, extend_to);
+ if (rc >= 0)
+ netfs_inode(inode)->remote_i_size = extend_to;
+ }
+
+unlock:
filemap_invalidate_unlock(inode->i_mapping);
out:
inode_unlock(inode);
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 9/9] netfs, cifs: Improve some debugging bits
2024-08-23 20:08 [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry David Howells
` (7 preceding siblings ...)
2024-08-23 20:08 ` [PATCH 8/9] cifs: Fix FALLOC_FL_PUNCH_HOLE support David Howells
@ 2024-08-23 20:08 ` David Howells
2024-08-24 11:56 ` [PATCH 10/9] netfs: Fix interaction of streaming writes with zero-point tracker David Howells
2024-08-24 14:09 ` (subset) [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry Christian Brauner
10 siblings, 0 replies; 14+ messages in thread
From: David Howells @ 2024-08-23 20:08 UTC (permalink / raw)
To: Christian Brauner, Steve French
Cc: David Howells, Pankaj Raghav, Paulo Alcantara, Jeff Layton,
Matthew Wilcox, netfs, linux-afs, linux-cifs, linux-nfs,
ceph-devel, v9fs, linux-erofs, linux-fsdevel, linux-mm,
linux-kernel
Improve some debugging bits:
(1) The netfslib _debug() macro doesn't need a newline in its format
string.
(2) Display the request debug ID and subrequest index in messages emitted
in smb2_adjust_credits() to make it easier to reference in traces.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Steve French <sfrench@samba.org>
cc: Paulo Alcantara <pc@manguebit.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cifs@vger.kernel.org
cc: netfs@lists.linux.dev
cc: linux-fsdevel@vger.kernel.org
---
fs/netfs/io.c | 2 +-
fs/smb/client/smb2ops.c | 8 +++++---
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/netfs/io.c b/fs/netfs/io.c
index 943128507af5..d6ada4eba744 100644
--- a/fs/netfs/io.c
+++ b/fs/netfs/io.c
@@ -270,7 +270,7 @@ static void netfs_reset_subreq_iter(struct netfs_io_request *rreq,
if (count == remaining)
return;
- _debug("R=%08x[%u] ITER RESUB-MISMATCH %zx != %zx-%zx-%llx %x\n",
+ _debug("R=%08x[%u] ITER RESUB-MISMATCH %zx != %zx-%zx-%llx %x",
rreq->debug_id, subreq->debug_index,
iov_iter_count(&subreq->io_iter), subreq->transferred,
subreq->len, rreq->i_size,
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 8fb68c47c218..2741f7d63fe7 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -316,7 +316,8 @@ smb2_adjust_credits(struct TCP_Server_Info *server,
cifs_trace_rw_credits_no_adjust_up);
trace_smb3_too_many_credits(server->CurrentMid,
server->conn_id, server->hostname, 0, credits->value - new_val, 0);
- cifs_server_dbg(VFS, "request has less credits (%d) than required (%d)",
+ cifs_server_dbg(VFS, "R=%x[%x] request has less credits (%d) than required (%d)",
+ subreq->rreq->debug_id, subreq->subreq.debug_index,
credits->value, new_val);
return -EOPNOTSUPP;
@@ -338,8 +339,9 @@ smb2_adjust_credits(struct TCP_Server_Info *server,
trace_smb3_reconnect_detected(server->CurrentMid,
server->conn_id, server->hostname, scredits,
credits->value - new_val, in_flight);
- cifs_server_dbg(VFS, "trying to return %d credits to old session\n",
- credits->value - new_val);
+ cifs_server_dbg(VFS, "R=%x[%x] trying to return %d credits to old session\n",
+ subreq->rreq->debug_id, subreq->subreq.debug_index,
+ credits->value - new_val);
return -EAGAIN;
}
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 10/9] netfs: Fix interaction of streaming writes with zero-point tracker
2024-08-23 20:08 [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry David Howells
` (8 preceding siblings ...)
2024-08-23 20:08 ` [PATCH 9/9] netfs, cifs: Improve some debugging bits David Howells
@ 2024-08-24 11:56 ` David Howells
2024-08-24 14:09 ` (subset) [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry Christian Brauner
10 siblings, 0 replies; 14+ messages in thread
From: David Howells @ 2024-08-24 11:56 UTC (permalink / raw)
Cc: dhowells, Christian Brauner, Steve French, Pankaj Raghav,
Paulo Alcantara, Jeff Layton, Matthew Wilcox, netfs, linux-afs,
linux-cifs, linux-nfs, ceph-devel, v9fs, linux-erofs,
linux-fsdevel, linux-mm, linux-kernel
When a folio that is marked for streaming write (dirty, but not uptodate,
with partial content specified in the private data) is written back, the
folio is effectively switched to the blank state upon completion of the
write. This means that if we want to read it in future, we need to reread
the whole folio.
However, if the folio is above the zero_point position, when it is read
back, it will just be cleared and the read skipped, leading to apparent
local corruption.
Fix this by increasing the zero_point to the end of the dirty data in the
folio when clearing the folio state after writeback. This is analogous to
the folio having ->release_folio() called upon it.
This was causing the config.log generated by configuring a cpython tree on
a cifs share to get corrupted because the scripts involved were appending
text to the file in small pieces.
Fixes: 288ace2f57c9 ("netfs: New writeback implementation")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Steve French <sfrench@samba.org>
cc: Paulo Alcantara <pc@manguebit.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cifs@vger.kernel.org
cc: netfs@lists.linux.dev
cc: linux-fsdevel@vger.kernel.org
---
fs/netfs/write_collect.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/fs/netfs/write_collect.c b/fs/netfs/write_collect.c
index 426cf87aaf2e..ae7a2043f670 100644
--- a/fs/netfs/write_collect.c
+++ b/fs/netfs/write_collect.c
@@ -33,6 +33,7 @@
int netfs_folio_written_back(struct folio *folio)
{
enum netfs_folio_trace why = netfs_folio_trace_clear;
+ struct netfs_inode *ictx = netfs_inode(folio->mapping->host);
struct netfs_folio *finfo;
struct netfs_group *group = NULL;
int gcount = 0;
@@ -41,6 +42,12 @@ int netfs_folio_written_back(struct folio *folio)
/* Streaming writes cannot be redirtied whilst under writeback,
* so discard the streaming record.
*/
+ unsigned long long fend;
+
+ fend = folio_pos(folio) + finfo->dirty_offset + finfo->dirty_len;
+ if (fend > ictx->zero_point)
+ ictx->zero_point = fend;
+
folio_detach_private(folio);
group = finfo->netfs_group;
gcount++;
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: (subset) [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry
2024-08-23 20:08 [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry David Howells
` (9 preceding siblings ...)
2024-08-24 11:56 ` [PATCH 10/9] netfs: Fix interaction of streaming writes with zero-point tracker David Howells
@ 2024-08-24 14:09 ` Christian Brauner
10 siblings, 0 replies; 14+ messages in thread
From: Christian Brauner @ 2024-08-24 14:09 UTC (permalink / raw)
To: David Howells, Steve French
Cc: Christian Brauner, Pankaj Raghav, Paulo Alcantara, Jeff Layton,
Matthew Wilcox, netfs, linux-afs, linux-cifs, linux-nfs,
ceph-devel, v9fs, linux-erofs, linux-fsdevel, linux-mm,
linux-kernel
On Fri, 23 Aug 2024 21:08:08 +0100, David Howells wrote:
> Firstly, there are some fixes for truncation, netfslib and afs that I
> discovered whilst trying Pankaj Raghav's minimum folio order patchset:
>
> (1) Fix truncate to make it honour AS_RELEASE_ALWAYS in a couple of places
> that got missed.
>
> (2) Fix duplicated editing of a partially invalidated folio in afs's
> post-setattr edit phase.
>
> [...]
Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes
[1/9] mm: Fix missing folio invalidation calls during truncation
https://git.kernel.org/vfs/vfs/c/0aa2e1b2fb7a
[2/9] afs: Fix post-setattr file edit to do truncation correctly
https://git.kernel.org/vfs/vfs/c/a74ee0e878e2
[3/9] netfs: Fix netfs_release_folio() to say no if folio dirty
https://git.kernel.org/vfs/vfs/c/7dfc8f0c6144
[4/9] netfs: Fix trimming of streaming-write folios in netfs_inval_folio()
https://git.kernel.org/vfs/vfs/c/cce6bfa6ca0e
[5/9] netfs: Fix missing iterator reset on retry of short read
https://git.kernel.org/vfs/vfs/c/950b03d0f664
[10/10] netfs: Fix interaction of streaming writes with zero-point tracker
https://git.kernel.org/vfs/vfs/c/e00e99ba6c6b
^ permalink raw reply [flat|nested] 14+ messages in thread