netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry
Subsystems (10)
David Howells
maintainercover netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry
Hi Christian, Steve,
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.
(3) Fix netfs_release_folio() to indicate that the folio is busy if the folio is dirty (as does iomap).
(4) Fix the trimming of a folio that contain streaming-write data when truncation occurs into or past that folio
Here's a patch to netfs for short reads:
(5) Fix netfslib's short read retry to reset the buffer iterator otherwise the wrong part of the buffer may get written on.
Here are a couple of fixes to DIO read handling and the retrying of reads, particularly in relation to cifs. They have both had some fixes to the fixes rolled in:
(6) Fix the missing credit renegotiation in cifs on the retrying of reads. The credits we had ended with the original read (or the last retry) and to perform a new read we need more credits otherwise the server can reject our read with EINVAL.
(7) Fix the handling of short DIO reads to avoid ENODATA when the read retry tries to access a portion of the file after the EOF.
Here's a fix for hole punching in cifs:
(8) Fix cifs FALLOC_FL_PUNCH_HOLE support as best I can. If it's going to punch a hole in dirty data in the pagecacne, invalidating that data may result in the EOF not being moved correctly. The set-zero and the eof-move RPC ops really need compounding to avoid third-party interference.
And finally, here's an adjustment to debugging statements:
(9) Adjust three debugging output statements. Not strictly a fix, so could be dropped. Including the subreq ID in some extra debug lines helps a bit, though.
The patches can also be found here:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=netfs-fixes
They're on top of part of Christian's vfs.fixes tree, including the partial reversion of the commit to replace PG_fscache.
Thanks,
David
David Howells (9):
mm: Fix missing folio invalidation calls during truncation
afs: Fix post-setattr file edit to do truncation correctly
netfs: Fix netfs_release_folio() to say no if folio dirty
netfs: Fix trimming of streaming-write folios in netfs_inval_folio()
netfs: Fix missing iterator reset on retry of short read
cifs: Fix lack of credit renegotiation on read retry netfs, cifs: Fix handling of short DIO read
cifs: Fix FALLOC_FL_PUNCH_HOLE support
netfs, cifs: Improve some debugging bits
fs/afs/inode.c | 11 ++++++--- fs/netfs/io.c | 22 +++++++++++------ fs/netfs/misc.c | 53 ++++++++++++++++++++++++++++------------ fs/smb/client/cifsglob.h | 1 + fs/smb/client/file.c | 37 +++++++++++++++++++++++++--- fs/smb/client/smb2ops.c | 32 +++++++++++++++++++++--- fs/smb/client/smb2pdu.c | 41 +++++++++++++++++++------------ fs/smb/client/trace.h | 1 + include/linux/netfs.h | 1 + mm/truncate.c | 4 +-- 10 files changed, 153 insertions(+), 50 deletions(-)
David Howells
maintainerPATCH 1/9 mm: Fix missing folio invalidation calls during truncation
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].
--- mm/truncate.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
mm/truncate.c +2 -2
@@ -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;
Re: [PATCH 1/9] mm: Fix missing folio invalidation calls during truncation
Quoted 9 lines · David Howells
When
AS_RELEASE_ALWAYSis set on a mapping, the ->release_folio() and ->invalidate_folio() calls should be invoked even ifPG_privateandPG_private_2aren'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 checkfolio_needs_release()instead.
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().
Re: [PATCH 1/9] mm: Fix missing folio invalidation calls during truncation
In reply to David HowellsQuoted 3 lines · David Howells
When
AS_RELEASE_ALWAYSis set on a mapping, the ->release_folio() and ->invalidate_folio() calls should be invoked even ifPG_privateandPG_private_2aren't set. This is used by netfslib to keep track of the
Should we update the comment in pagemap?
include/linux/pagemap.h +2 -1
@@ -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 */
Quoted 48 lines
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 checkfolio_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 <[email protected]>cc Matthew Wilcox (Oracle) <[email protected]>cc Pankaj Raghav <[email protected]>cc Jeff Layton <[email protected]>cc Marc Dionne <[email protected]>--- mm/truncate.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)mm/truncate.c +2
-2@@ -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;
Signature · 1 line
Pankaj Raghav
David Howells
maintainerPATCH 2/9 afs: Fix post-setattr file edit to do truncation correctly
In reply to David HowellsAt 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.
--- fs/afs/inode.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
fs/afs/inode.c +8 -3
@@ -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); }
David Howells
maintainerPATCH 3/9 netfs: Fix netfs_release_folio() to say no if folio dirty
In reply to David HowellsFix 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].
--- fs/netfs/misc.c | 3 +++ 1 file changed, 3 insertions(+)
fs/netfs/misc.c +3 -0
@@ -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;
David Howells
maintainerPATCH 4/9 netfs: Fix trimming of streaming-write folios in netfs_inval_folio()
In reply to David HowellsWhen 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.
--- fs/netfs/misc.c | 50 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 15 deletions(-)
fs/netfs/misc.c +35 -15
@@ -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;
David Howells
maintainerPATCH 5/9 netfs: Fix missing iterator reset on retry of short read
In reply to David HowellsFix netfs_rreq_perform_resubmissions() to reset before retrying a short read, otherwise the wrong part of the output buffer will be used.
--- fs/netfs/io.c | 1 + 1 file changed, 1 insertion(+)
fs/netfs/io.c +1 -0
@@ -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); } }
David Howells
maintainerPATCH 6/9 cifs: Fix lack of credit renegotiation on read retry
In reply to David HowellsWhen 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.
--- 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(-)
fs/netfs/io.c +2 -0
@@ -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); }
fs/smb/client/cifsglob.h +1 -0
@@ -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;
fs/smb/client/file.c +33 -4
@@ -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);
fs/smb/client/smb2ops.c +1 -1
@@ -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)
fs/smb/client/smb2pdu.c +17 -11
@@ -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:
fs/smb/client/trace.h +1 -0
@@ -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" ) \
David Howells
maintainerPATCH 7/9 netfs, cifs: Fix handling of short DIO read
In reply to David HowellsShort 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.
--- fs/netfs/io.c | 17 +++++++++++------ fs/smb/client/smb2pdu.c | 13 +++++++++---- include/linux/netfs.h | 1 + 3 files changed, 21 insertions(+), 10 deletions(-)
fs/netfs/io.c +11 -6
@@ -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. */
fs/smb/client/smb2pdu.c +9 -4
@@ -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,
include/linux/netfs.h +1 -0
@@ -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 {
David Howells
maintainerPATCH 8/9 cifs: Fix FALLOC_FL_PUNCH_HOLE support
In reply to David HowellsThe 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.
--- fs/smb/client/smb2ops.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
fs/smb/client/smb2ops.c +22 -0
@@ -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);
David Howells
maintainerPATCH 9/9 netfs, cifs: Improve some debugging bits
In reply to David HowellsImprove 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.
--- fs/netfs/io.c | 2 +- fs/smb/client/smb2ops.c | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-)
fs/netfs/io.c +1 -1
@@ -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,
fs/smb/client/smb2ops.c +5 -3
@@ -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 ; }
David Howells
maintainer[PATCH 10/9] netfs: Fix interaction of streaming writes with zero-point tracker
In reply to David HowellsWhen 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.
--- fs/netfs/write_collect.c | 7 +++++++ 1 file changed, 7 insertions(+)
fs/netfs/write_collect.c +7 -0
@@ -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++;
Re: (subset) [PATCH 0/9] netfs, cifs: Combined repost of fixes for truncation, DIO read and read-retry
In reply to David HowellsQuoted 10 lines · David Howells
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_ALWAYSin 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