* [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error
@ 2026-01-20 6:21 nspmangalore
2026-01-20 6:21 ` [PATCH 2/4] netfs: avoid double increment of retry_count in subreq nspmangalore
` (4 more replies)
0 siblings, 5 replies; 18+ messages in thread
From: nspmangalore @ 2026-01-20 6:21 UTC (permalink / raw)
To: linux-cifs, smfrench, pc, bharathsm, dhowells; +Cc: Shyam Prasad N
From: Shyam Prasad N <sprasad@microsoft.com>
The *_subreq_terminated functions today only process the NEED_RETRY
flag when the subreq was successful or failed with EAGAIN error.
However, there could be other retriable errors for network filesystems.
Avoid this by processing the NEED_RETRY irrespective of the error
code faced by the subreq. If it was specifically marked for retry,
the error code must not matter.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
fs/netfs/read_collect.c | 6 ++++--
fs/netfs/read_retry.c | 4 ++--
fs/netfs/write_collect.c | 8 ++++----
fs/netfs/write_issue.c | 1 +
4 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/fs/netfs/read_collect.c b/fs/netfs/read_collect.c
index a95e7aadafd07..743830a149bb6 100644
--- a/fs/netfs/read_collect.c
+++ b/fs/netfs/read_collect.c
@@ -547,13 +547,15 @@ void netfs_read_subreq_terminated(struct netfs_io_subrequest *subreq)
}
if (unlikely(subreq->error < 0)) {
- trace_netfs_failure(rreq, subreq, subreq->error, netfs_fail_read);
if (subreq->source == NETFS_READ_FROM_CACHE) {
netfs_stat(&netfs_n_rh_read_failed);
__set_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
} else {
netfs_stat(&netfs_n_rh_download_failed);
- __set_bit(NETFS_SREQ_FAILED, &subreq->flags);
+ if (!test_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags)) {
+ __set_bit(NETFS_SREQ_FAILED, &subreq->flags);
+ trace_netfs_failure(rreq, subreq, subreq->error, netfs_fail_read);
+ }
}
trace_netfs_rreq(rreq, netfs_rreq_trace_set_pause);
set_bit(NETFS_RREQ_PAUSE, &rreq->flags);
diff --git a/fs/netfs/read_retry.c b/fs/netfs/read_retry.c
index b99e84a8170af..7793ba5e3e8fc 100644
--- a/fs/netfs/read_retry.c
+++ b/fs/netfs/read_retry.c
@@ -12,6 +12,7 @@
static void netfs_reissue_read(struct netfs_io_request *rreq,
struct netfs_io_subrequest *subreq)
{
+ subreq->error = 0;
__clear_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags);
__set_bit(NETFS_SREQ_IN_PROGRESS, &subreq->flags);
netfs_stat(&netfs_n_rh_retry_read_subreq);
@@ -242,8 +243,7 @@ static void netfs_retry_read_subrequests(struct netfs_io_request *rreq)
subreq = list_next_entry(subreq, rreq_link);
abandon:
list_for_each_entry_from(subreq, &stream->subrequests, rreq_link) {
- if (!subreq->error &&
- !test_bit(NETFS_SREQ_FAILED, &subreq->flags) &&
+ if (!test_bit(NETFS_SREQ_FAILED, &subreq->flags) &&
!test_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags))
continue;
subreq->error = -ENOMEM;
diff --git a/fs/netfs/write_collect.c b/fs/netfs/write_collect.c
index cbf3d9194c7bf..61eab34ea67ef 100644
--- a/fs/netfs/write_collect.c
+++ b/fs/netfs/write_collect.c
@@ -492,11 +492,11 @@ void netfs_write_subrequest_terminated(void *_op, ssize_t transferred_or_error)
if (IS_ERR_VALUE(transferred_or_error)) {
subreq->error = transferred_or_error;
- if (subreq->error == -EAGAIN)
- set_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
- else
+ /* if need retry is set, error should not matter */
+ if (!test_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags)) {
set_bit(NETFS_SREQ_FAILED, &subreq->flags);
- trace_netfs_failure(wreq, subreq, transferred_or_error, netfs_fail_write);
+ trace_netfs_failure(wreq, subreq, transferred_or_error, netfs_fail_write);
+ }
switch (subreq->source) {
case NETFS_WRITE_TO_CACHE:
diff --git a/fs/netfs/write_issue.c b/fs/netfs/write_issue.c
index dd8743bc8d7fe..34894da5a23ec 100644
--- a/fs/netfs/write_issue.c
+++ b/fs/netfs/write_issue.c
@@ -250,6 +250,7 @@ void netfs_reissue_write(struct netfs_io_stream *stream,
iov_iter_truncate(&subreq->io_iter, size);
subreq->retry_count++;
+ subreq->error = 0;
__clear_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags);
__set_bit(NETFS_SREQ_IN_PROGRESS, &subreq->flags);
netfs_stat(&netfs_n_wh_retry_write_subreq);
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/4] netfs: avoid double increment of retry_count in subreq
2026-01-20 6:21 [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error nspmangalore
@ 2026-01-20 6:21 ` nspmangalore
2026-01-21 23:23 ` David Howells
2026-01-20 6:21 ` [PATCH 3/4] cifs: Initialize cur_sleep value if not already done nspmangalore
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: nspmangalore @ 2026-01-20 6:21 UTC (permalink / raw)
To: linux-cifs, smfrench, pc, bharathsm, dhowells; +Cc: Shyam Prasad N
From: Shyam Prasad N <sprasad@microsoft.com>
This change fixes the instance of double incrementing of
retry_count. The increment of this count already happens
when netfs_reissue_write gets called. Incrementing this
value before is not necessary.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
fs/netfs/write_retry.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/netfs/write_retry.c b/fs/netfs/write_retry.c
index fc9c3e0d34d81..29489a23a2209 100644
--- a/fs/netfs/write_retry.c
+++ b/fs/netfs/write_retry.c
@@ -98,7 +98,6 @@ static void netfs_retry_write_stream(struct netfs_io_request *wreq,
subreq->start = start;
subreq->len = len;
__clear_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
- subreq->retry_count++;
trace_netfs_sreq(subreq, netfs_sreq_trace_retry);
/* Renegotiate max_len (wsize) */
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/4] cifs: Initialize cur_sleep value if not already done
2026-01-20 6:21 [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error nspmangalore
2026-01-20 6:21 ` [PATCH 2/4] netfs: avoid double increment of retry_count in subreq nspmangalore
@ 2026-01-20 6:21 ` nspmangalore
2026-01-21 23:28 ` David Howells
2026-01-20 6:21 ` [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths nspmangalore
` (2 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: nspmangalore @ 2026-01-20 6:21 UTC (permalink / raw)
To: linux-cifs, smfrench, pc, bharathsm, dhowells; +Cc: Shyam Prasad N
From: Shyam Prasad N <sprasad@microsoft.com>
A value of 0 for cur_sleep is not a valid one. By checking for
a zero value and explicitly setting it to 1, we avoid the added
dependency on the callers.
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
fs/smb/client/smb2ops.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index a16ded46b5a26..a7e06d175f899 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -2774,6 +2774,8 @@ bool smb2_should_replay(struct cifs_tcon *tcon,
return false;
if (tcon->retry || (*pretries)++ < tcon->ses->server->retrans) {
+ if (!(*pcur_sleep))
+ (*pcur_sleep) = 1;
msleep(*pcur_sleep);
(*pcur_sleep) = ((*pcur_sleep) << 1);
if ((*pcur_sleep) > CIFS_MAX_SLEEP)
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths
2026-01-20 6:21 [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error nspmangalore
2026-01-20 6:21 ` [PATCH 2/4] netfs: avoid double increment of retry_count in subreq nspmangalore
2026-01-20 6:21 ` [PATCH 3/4] cifs: Initialize cur_sleep value if not already done nspmangalore
@ 2026-01-20 6:21 ` nspmangalore
2026-01-20 11:32 ` kernel test robot
` (3 more replies)
2026-01-21 23:08 ` [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error David Howells
2026-01-21 23:18 ` David Howells
4 siblings, 4 replies; 18+ messages in thread
From: nspmangalore @ 2026-01-20 6:21 UTC (permalink / raw)
To: linux-cifs, smfrench, pc, bharathsm, dhowells; +Cc: Shyam Prasad N
From: Shyam Prasad N <sprasad@microsoft.com>
Today in most other code paths in cifs.ko, the decision of whether
to retry a command depends on two mount options: retrans and hard.
However, the read/write code paths diverged from this and would only
retry if the error returned was -EAGAIN. However, there are other
replayable errors in cifs.ko, for which is_replayable_errors helper
was written. This change makes read/write codepaths consistent with
other code-paths.
This change also does the following:
1. The SMB2 read/write code diverged significantly (presumably since
they were changed during netfs refactor at different times). This
changes the response verification logic to be consistent.
2. Moves the netfs tracepoints to slightly different locations in order
to make debugging easier.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
fs/smb/client/cifsglob.h | 2 ++
fs/smb/client/smb2pdu.c | 70 +++++++++++++++++++++++++++++++---------
2 files changed, 56 insertions(+), 16 deletions(-)
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 3eca5bfb70303..f6ebd3fd176d7 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1507,6 +1507,8 @@ struct cifs_io_subrequest {
int result;
bool have_xid;
bool replay;
+ unsigned int retries; /* number of retries so far */
+ unsigned int cur_sleep; /* time to sleep before replay */
struct kvec iov[2];
struct TCP_Server_Info *server;
#ifdef CONFIG_CIFS_SMB_DIRECT
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 5d57c895ca37a..89f728392a734 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -4616,17 +4616,19 @@ smb2_readv_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
case MID_RESPONSE_RECEIVED:
credits.value = le16_to_cpu(shdr->CreditRequest);
credits.instance = server->reconnect_instance;
- /* result already set, check signature */
- if (server->sign && !mid->decrypted) {
- int rc;
+ rdata->result = smb2_check_receive(mid, server, 0);
+ if (rdata->result != 0) {
+ rdata->subreq.error = rdata->result;
+ if (is_replayable_error(rdata->result)) {
+ trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_req_submitted);
+ __set_bit(NETFS_SREQ_NEED_RETRY, &rdata->subreq.flags);
+ } else {
+ trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_bad);
+ }
+ break;
+ } else
+ trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress);
- iov_iter_truncate(&rqst.rq_iter, rdata->got_bytes);
- rc = smb2_verify_signature(&rqst, server);
- if (rc)
- cifs_tcon_dbg(VFS, "SMB signature verification returned error = %d\n",
- rc);
- }
- /* FIXME: should this be counted toward the initiating task? */
task_io_account_read(rdata->got_bytes);
cifs_stats_bytes_read(tcon, rdata->got_bytes);
break;
@@ -4748,7 +4750,7 @@ smb2_async_readv(struct cifs_io_subrequest *rdata)
rc = smb2_new_read_req(
(void **) &buf, &total_len, &io_parms, rdata, 0, 0);
if (rc)
- return rc;
+ goto out;
if (smb3_encryption_required(io_parms.tcon))
flags |= CIFS_TRANSFORM_REQ;
@@ -4795,6 +4797,17 @@ smb2_async_readv(struct cifs_io_subrequest *rdata)
async_readv_out:
cifs_small_buf_release(buf);
+
+out:
+ /* if the send error is retryable, let netfs know about it */
+ if (is_replayable_error(rc) &&
+ smb2_should_replay(tcon,
+ &rdata->retries,
+ &rdata->cur_sleep)) {
+ trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_retry_needed);
+ __set_bit(NETFS_SREQ_NEED_RETRY, &rdata->subreq.flags);
+ }
+
return rc;
}
@@ -4908,14 +4921,20 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
switch (mid->mid_state) {
case MID_RESPONSE_RECEIVED:
- trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_progress);
credits.value = le16_to_cpu(rsp->hdr.CreditRequest);
credits.instance = server->reconnect_instance;
result = smb2_check_receive(mid, server, 0);
if (result != 0) {
- trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_bad);
+ if (is_replayable_error(result)) {
+ trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_req_submitted);
+ __set_bit(NETFS_SREQ_NEED_RETRY, &wdata->subreq.flags);
+ } else {
+ wdata->subreq.error = result;
+ trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_bad);
+ }
break;
- }
+ } else
+ trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_progress);
written = le32_to_cpu(rsp->DataLength);
/*
@@ -4930,7 +4949,7 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
cifs_stats_bytes_written(tcon, written);
if (written < wdata->subreq.len) {
- wdata->result = -ENOSPC;
+ result = -ENOSPC;
} else if (written > 0) {
wdata->subreq.len = written;
__set_bit(NETFS_SREQ_MADE_PROGRESS, &wdata->subreq.flags);
@@ -4972,6 +4991,7 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
}
#endif
if (result) {
+ wdata->result = result;
cifs_stats_fail_inc(tcon, SMB2_WRITE_HE);
trace_smb3_write_err(wdata->rreq->debug_id,
wdata->subreq.debug_index,
@@ -4994,6 +5014,14 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
server->credits, server->in_flight,
0, cifs_trace_rw_credits_write_response_clear);
wdata->credits.value = 0;
+
+ /* see if we need to retry */
+ if (is_replayable_error(wdata->result) &&
+ smb2_should_replay(tcon,
+ &wdata->retries,
+ &wdata->cur_sleep))
+ wdata->replay = true;
+
cifs_write_subrequest_terminated(wdata, result ?: written);
release_mid(server, mid);
trace_smb3_rw_credits(rreq_debug_id, subreq_debug_index, 0,
@@ -5112,7 +5140,7 @@ smb2_async_writev(struct cifs_io_subrequest *wdata)
}
#endif
- if (wdata->subreq.retry_count > 0)
+ if (wdata->replay)
smb2_set_replay(server, &rqst);
cifs_dbg(FYI, "async write at %llu %u bytes iter=%zx\n",
@@ -5159,6 +5187,16 @@ smb2_async_writev(struct cifs_io_subrequest *wdata)
async_writev_out:
cifs_small_buf_release(req);
out:
+ /* if the send error is retryable, let netfs know about it */
+ if (is_replayable_error(rc) &&
+ smb2_should_replay(tcon,
+ &wdata->retries,
+ &wdata->cur_sleep)) {
+ wdata->replay = true;
+ trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_retry_needed);
+ __set_bit(NETFS_SREQ_NEED_RETRY, &wdata->subreq.flags);
+ }
+
if (rc) {
trace_smb3_rw_credits(wdata->rreq->debug_id,
wdata->subreq.debug_index,
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths
2026-01-20 6:21 ` [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths nspmangalore
@ 2026-01-20 11:32 ` kernel test robot
2026-01-20 17:03 ` David Howells
` (2 subsequent siblings)
3 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2026-01-20 11:32 UTC (permalink / raw)
To: nspmangalore, linux-cifs, smfrench, pc, bharathsm, dhowells
Cc: oe-kbuild-all, Shyam Prasad N
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on cifs/for-next]
[also build test WARNING on brauner-vfs/vfs.all linus/master v6.19-rc6 next-20260119]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/nspmangalore-gmail-com/netfs-avoid-double-increment-of-retry_count-in-subreq/20260120-142802
base: git://git.samba.org/sfrench/cifs-2.6.git for-next
patch link: https://lore.kernel.org/r/20260120062152.628822-4-sprasad%40microsoft.com
patch subject: [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths
config: parisc-defconfig (https://download.01.org/0day-ci/archive/20260120/202601201945.Kjfrc2gQ-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 15.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260120/202601201945.Kjfrc2gQ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202601201945.Kjfrc2gQ-lkp@intel.com/
All warnings (new ones prefixed by >>):
fs/smb/client/smb2pdu.c: In function 'smb2_readv_callback':
>> fs/smb/client/smb2pdu.c:4599:25: warning: variable 'rqst' set but not used [-Wunused-but-set-variable]
4599 | struct smb_rqst rqst = { .rq_iov = &rdata->iov[0], .rq_nvec = 1 };
| ^~~~
vim +/rqst +4599 fs/smb/client/smb2pdu.c
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4585
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4586 static void
87fba18abbb843 fs/smb/client/smb2pdu.c David Howells 2025-10-03 4587 smb2_readv_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4588 {
753b67eb630db3 fs/smb/client/smb2pdu.c David Howells 2022-03-09 4589 struct cifs_io_subrequest *rdata = mid->callback_data;
1da29f2c39b67b fs/smb/client/smb2pdu.c David Howells 2024-08-22 4590 struct netfs_inode *ictx = netfs_inode(rdata->rreq->inode);
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4591 struct cifs_tcon *tcon = tlink_tcon(rdata->req->cfile->tlink);
87fba18abbb843 fs/smb/client/smb2pdu.c David Howells 2025-10-03 4592 struct smb2_hdr *shdr = (struct smb2_hdr *)rdata->iov[0].iov_base;
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4593 struct cifs_credits credits = {
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4594 .value = 0,
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4595 .instance = 0,
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4596 .rreq_debug_id = rdata->rreq->debug_id,
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4597 .rreq_debug_index = rdata->subreq.debug_index,
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4598 };
83bfbd0bb9025f fs/smb/client/smb2pdu.c David Howells 2025-09-05 @4599 struct smb_rqst rqst = { .rq_iov = &rdata->iov[0], .rq_nvec = 1 };
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4600 unsigned int rreq_debug_id = rdata->rreq->debug_id;
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4601 unsigned int subreq_debug_index = rdata->subreq.debug_index;
023fc150a39ffe fs/cifs/smb2pdu.c David Howells 2023-04-18 4602
023fc150a39ffe fs/cifs/smb2pdu.c David Howells 2023-04-18 4603 if (rdata->got_bytes) {
ab58fbdeebc7f9 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4604 rqst.rq_iter = rdata->subreq.io_iter;
9ee04875ae7341 fs/cifs/smb2pdu.c Yang Li 2023-05-05 4605 }
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4606
87fba18abbb843 fs/smb/client/smb2pdu.c David Howells 2025-10-03 4607 WARN_ONCE(rdata->server != server,
352d96f3acc6e0 fs/cifs/smb2pdu.c Aurelien Aptel 2020-05-31 4608 "rdata server %p != mid server %p",
87fba18abbb843 fs/smb/client/smb2pdu.c David Howells 2025-10-03 4609 rdata->server, server);
352d96f3acc6e0 fs/cifs/smb2pdu.c Aurelien Aptel 2020-05-31 4610
6a5dcd487791e0 fs/smb/client/smb2pdu.c David Howells 2024-08-22 4611 cifs_dbg(FYI, "%s: mid=%llu state=%d result=%d bytes=%zu/%zu\n",
f96637be081141 fs/cifs/smb2pdu.c Joe Perches 2013-05-04 4612 __func__, mid->mid, mid->mid_state, rdata->result,
ee4cdf7ba857a8 fs/smb/client/smb2pdu.c David Howells 2024-07-02 4613 rdata->got_bytes, rdata->subreq.len - rdata->subreq.transferred);
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4614
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4615 switch (mid->mid_state) {
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4616 case MID_RESPONSE_RECEIVED:
34f4deb7c56c6f fs/cifs/smb2pdu.c Pavel Shilovsky 2019-01-16 4617 credits.value = le16_to_cpu(shdr->CreditRequest);
34f4deb7c56c6f fs/cifs/smb2pdu.c Pavel Shilovsky 2019-01-16 4618 credits.instance = server->reconnect_instance;
b5649b5d382022 fs/smb/client/smb2pdu.c Shyam Prasad N 2026-01-20 4619 rdata->result = smb2_check_receive(mid, server, 0);
b5649b5d382022 fs/smb/client/smb2pdu.c Shyam Prasad N 2026-01-20 4620 if (rdata->result != 0) {
b5649b5d382022 fs/smb/client/smb2pdu.c Shyam Prasad N 2026-01-20 4621 rdata->subreq.error = rdata->result;
b5649b5d382022 fs/smb/client/smb2pdu.c Shyam Prasad N 2026-01-20 4622 if (is_replayable_error(rdata->result)) {
b5649b5d382022 fs/smb/client/smb2pdu.c Shyam Prasad N 2026-01-20 4623 trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_req_submitted);
b5649b5d382022 fs/smb/client/smb2pdu.c Shyam Prasad N 2026-01-20 4624 __set_bit(NETFS_SREQ_NEED_RETRY, &rdata->subreq.flags);
b5649b5d382022 fs/smb/client/smb2pdu.c Shyam Prasad N 2026-01-20 4625 } else {
b5649b5d382022 fs/smb/client/smb2pdu.c Shyam Prasad N 2026-01-20 4626 trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_bad);
3c1bf7e48e9e46 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4627 }
b5649b5d382022 fs/smb/client/smb2pdu.c Shyam Prasad N 2026-01-20 4628 break;
b5649b5d382022 fs/smb/client/smb2pdu.c Shyam Prasad N 2026-01-20 4629 } else
b5649b5d382022 fs/smb/client/smb2pdu.c Shyam Prasad N 2026-01-20 4630 trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress);
b5649b5d382022 fs/smb/client/smb2pdu.c Shyam Prasad N 2026-01-20 4631
34a54d617785e5 fs/cifs/smb2pdu.c Pavel Shilovsky 2014-07-10 4632 task_io_account_read(rdata->got_bytes);
34a54d617785e5 fs/cifs/smb2pdu.c Pavel Shilovsky 2014-07-10 4633 cifs_stats_bytes_read(tcon, rdata->got_bytes);
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4634 break;
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4635 case MID_REQUEST_SUBMITTED:
90b3ccf514578c fs/smb/client/smb2pdu.c David Howells 2025-07-01 4636 trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_req_submitted);
90b3ccf514578c fs/smb/client/smb2pdu.c David Howells 2025-07-01 4637 goto do_retry;
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4638 case MID_RETRY_NEEDED:
90b3ccf514578c fs/smb/client/smb2pdu.c David Howells 2025-07-01 4639 trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_retry_needed);
90b3ccf514578c fs/smb/client/smb2pdu.c David Howells 2025-07-01 4640 do_retry:
ee4cdf7ba857a8 fs/smb/client/smb2pdu.c David Howells 2024-07-02 4641 __set_bit(NETFS_SREQ_NEED_RETRY, &rdata->subreq.flags);
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4642 rdata->result = -EAGAIN;
d913ed17f0a7d7 fs/cifs/smb2pdu.c Pavel Shilovsky 2014-07-10 4643 if (server->sign && rdata->got_bytes)
d913ed17f0a7d7 fs/cifs/smb2pdu.c Pavel Shilovsky 2014-07-10 4644 /* reset bytes number since we can not check a sign */
d913ed17f0a7d7 fs/cifs/smb2pdu.c Pavel Shilovsky 2014-07-10 4645 rdata->got_bytes = 0;
d913ed17f0a7d7 fs/cifs/smb2pdu.c Pavel Shilovsky 2014-07-10 4646 /* FIXME: should this be counted toward the initiating task? */
d913ed17f0a7d7 fs/cifs/smb2pdu.c Pavel Shilovsky 2014-07-10 4647 task_io_account_read(rdata->got_bytes);
d913ed17f0a7d7 fs/cifs/smb2pdu.c Pavel Shilovsky 2014-07-10 4648 cifs_stats_bytes_read(tcon, rdata->got_bytes);
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4649 break;
0fd1d37b0501ef fs/cifs/smb2pdu.c Pavel Shilovsky 2019-01-15 4650 case MID_RESPONSE_MALFORMED:
90b3ccf514578c fs/smb/client/smb2pdu.c David Howells 2025-07-01 4651 trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_malformed);
34f4deb7c56c6f fs/cifs/smb2pdu.c Pavel Shilovsky 2019-01-16 4652 credits.value = le16_to_cpu(shdr->CreditRequest);
34f4deb7c56c6f fs/cifs/smb2pdu.c Pavel Shilovsky 2019-01-16 4653 credits.instance = server->reconnect_instance;
f80ac7eda1cf52 fs/smb/client/smb2pdu.c David Howells 2025-10-24 4654 rdata->result = smb_EIO(smb_eio_trace_read_rsp_malformed);
90b3ccf514578c fs/smb/client/smb2pdu.c David Howells 2025-07-01 4655 break;
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4656 default:
90b3ccf514578c fs/smb/client/smb2pdu.c David Howells 2025-07-01 4657 trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_unknown);
f80ac7eda1cf52 fs/smb/client/smb2pdu.c David Howells 2025-10-24 4658 rdata->result = smb_EIO1(smb_eio_trace_read_mid_state_unknown,
f80ac7eda1cf52 fs/smb/client/smb2pdu.c David Howells 2025-10-24 4659 mid->mid_state);
90b3ccf514578c fs/smb/client/smb2pdu.c David Howells 2025-07-01 4660 break;
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4661 }
bd3dcc6a22a918 fs/cifs/smb2pdu.c Long Li 2017-11-22 4662 #ifdef CONFIG_CIFS_SMB_DIRECT
bd3dcc6a22a918 fs/cifs/smb2pdu.c Long Li 2017-11-22 4663 /*
e9f49feefb4b13 fs/smb/client/smb2pdu.c Shen Lichuan 2024-09-25 4664 * If this rdata has a memory registered, the MR can be freed
bd3dcc6a22a918 fs/cifs/smb2pdu.c Long Li 2017-11-22 4665 * MR needs to be freed as soon as I/O finishes to prevent deadlock
bd3dcc6a22a918 fs/cifs/smb2pdu.c Long Li 2017-11-22 4666 * because they have limited number and are used for future I/Os
bd3dcc6a22a918 fs/cifs/smb2pdu.c Long Li 2017-11-22 4667 */
bd3dcc6a22a918 fs/cifs/smb2pdu.c Long Li 2017-11-22 4668 if (rdata->mr) {
bd3dcc6a22a918 fs/cifs/smb2pdu.c Long Li 2017-11-22 4669 smbd_deregister_mr(rdata->mr);
bd3dcc6a22a918 fs/cifs/smb2pdu.c Long Li 2017-11-22 4670 rdata->mr = NULL;
bd3dcc6a22a918 fs/cifs/smb2pdu.c Long Li 2017-11-22 4671 }
bd3dcc6a22a918 fs/cifs/smb2pdu.c Long Li 2017-11-22 4672 #endif
082aaa8700415f fs/cifs/smb2pdu.c Pavel Shilovsky 2019-01-18 4673 if (rdata->result && rdata->result != -ENODATA) {
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4674 cifs_stats_fail_inc(tcon, SMB2_READ_HE);
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4675 trace_smb3_read_err(rdata->rreq->debug_id,
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4676 rdata->subreq.debug_index,
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4677 rdata->xid,
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4678 rdata->req->cfile->fid.persistent_fid,
6a5dcd487791e0 fs/smb/client/smb2pdu.c David Howells 2024-08-22 4679 tcon->tid, tcon->ses->Suid,
6a5dcd487791e0 fs/smb/client/smb2pdu.c David Howells 2024-08-22 4680 rdata->subreq.start + rdata->subreq.transferred,
ee4cdf7ba857a8 fs/smb/client/smb2pdu.c David Howells 2024-07-02 4681 rdata->subreq.len - rdata->subreq.transferred,
6a5dcd487791e0 fs/smb/client/smb2pdu.c David Howells 2024-08-22 4682 rdata->result);
7d42e72fe8ee5a fs/cifs/smb2pdu.c Pavel Shilovsky 2019-01-25 4683 } else
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4684 trace_smb3_read_done(rdata->rreq->debug_id,
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4685 rdata->subreq.debug_index,
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4686 rdata->xid,
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4687 rdata->req->cfile->fid.persistent_fid,
7d42e72fe8ee5a fs/cifs/smb2pdu.c Pavel Shilovsky 2019-01-25 4688 tcon->tid, tcon->ses->Suid,
6a5dcd487791e0 fs/smb/client/smb2pdu.c David Howells 2024-08-22 4689 rdata->subreq.start + rdata->subreq.transferred,
6a5dcd487791e0 fs/smb/client/smb2pdu.c David Howells 2024-08-22 4690 rdata->got_bytes);
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4691
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4692 if (rdata->result == -ENODATA) {
1da29f2c39b67b fs/smb/client/smb2pdu.c David Howells 2024-08-22 4693 __set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
1da29f2c39b67b fs/smb/client/smb2pdu.c David Howells 2024-08-22 4694 rdata->result = 0;
1da29f2c39b67b fs/smb/client/smb2pdu.c David Howells 2024-08-22 4695 } else {
ee4cdf7ba857a8 fs/smb/client/smb2pdu.c David Howells 2024-07-02 4696 size_t trans = rdata->subreq.transferred + rdata->got_bytes;
ee4cdf7ba857a8 fs/smb/client/smb2pdu.c David Howells 2024-07-02 4697 if (trans < rdata->subreq.len &&
4ae4dde6f34a41 fs/smb/client/smb2pdu.c David Howells 2025-12-03 4698 rdata->subreq.start + trans >= ictx->remote_i_size) {
1da29f2c39b67b fs/smb/client/smb2pdu.c David Howells 2024-08-22 4699 __set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4700 rdata->result = 0;
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4701 }
e2d46f2ec33253 fs/smb/client/smb2pdu.c David Howells 2024-12-16 4702 if (rdata->got_bytes)
4acb665cf4f3e5 fs/smb/client/smb2pdu.c David Howells 2024-12-13 4703 __set_bit(NETFS_SREQ_MADE_PROGRESS, &rdata->subreq.flags);
1da29f2c39b67b fs/smb/client/smb2pdu.c David Howells 2024-08-22 4704 }
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4705 trace_smb3_rw_credits(rreq_debug_id, subreq_debug_index, rdata->credits.value,
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4706 server->credits, server->in_flight,
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4707 0, cifs_trace_rw_credits_read_response_clear);
3ee1a1fc398199 fs/smb/client/smb2pdu.c David Howells 2023-10-06 4708 rdata->credits.value = 0;
360157829ee3db fs/smb/client/smb2pdu.c David Howells 2024-12-16 4709 rdata->subreq.error = rdata->result;
ee4cdf7ba857a8 fs/smb/client/smb2pdu.c David Howells 2024-07-02 4710 rdata->subreq.transferred += rdata->got_bytes;
ee4cdf7ba857a8 fs/smb/client/smb2pdu.c David Howells 2024-07-02 4711 trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress);
e2d46f2ec33253 fs/smb/client/smb2pdu.c David Howells 2024-12-16 4712 netfs_read_subreq_terminated(&rdata->subreq);
87fba18abbb843 fs/smb/client/smb2pdu.c David Howells 2025-10-03 4713 release_mid(server, mid);
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4714 trace_smb3_rw_credits(rreq_debug_id, subreq_debug_index, 0,
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4715 server->credits, server->in_flight,
519be989717c5b fs/smb/client/smb2pdu.c David Howells 2024-05-23 4716 credits.value, cifs_trace_rw_credits_read_response_add);
34f4deb7c56c6f fs/cifs/smb2pdu.c Pavel Shilovsky 2019-01-16 4717 add_credits(server, &credits, 0);
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4718 }
09a4707e763824 fs/cifs/smb2pdu.c Pavel Shilovsky 2012-09-18 4719
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths
2026-01-20 6:21 ` [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths nspmangalore
2026-01-20 11:32 ` kernel test robot
@ 2026-01-20 17:03 ` David Howells
2026-01-21 4:45 ` Shyam Prasad N
2026-01-21 1:56 ` Steve French
2026-01-21 23:36 ` David Howells
3 siblings, 1 reply; 18+ messages in thread
From: David Howells @ 2026-01-20 17:03 UTC (permalink / raw)
To: nspmangalore
Cc: dhowells, linux-cifs, smfrench, pc, bharathsm, Shyam Prasad N
Hi Shyam,
Some immediate thoughts for you.
nspmangalore@gmail.com wrote:
> + break;
> + } else
> + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress);
The 'else' is superfluous, given the 'break'.
> + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_retry_needed);
Btw, feel free to add more trace values if you want to distinguish 'retries'
from 'replays' if that makes sense.
> break;
> - }
> + } else
> + trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_progress);
Superfluous 'else' again.
> - wdata->result = -ENOSPC;
> + result = -ENOSPC;
I wonder if wdata->result is redundant. Can wdata->subreq.error be used
instead (though that's unrelated to the subject of this patch).
David
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths
2026-01-20 6:21 ` [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths nspmangalore
2026-01-20 11:32 ` kernel test robot
2026-01-20 17:03 ` David Howells
@ 2026-01-21 1:56 ` Steve French
2026-01-21 4:46 ` Shyam Prasad N
2026-01-21 23:36 ` David Howells
3 siblings, 1 reply; 18+ messages in thread
From: Steve French @ 2026-01-21 1:56 UTC (permalink / raw)
To: nspmangalore; +Cc: linux-cifs, pc, bharathsm, dhowells, Shyam Prasad N
Checkpatch flagged this with minor warning. Let me know if you do
updated version of it
0004-cifs-make-retry-logic-in-read-write-path-consistent-.patch
------------------------------------------------------------------------
WARNING: else is not generally useful after a break or return
#64: FILE: fs/smb/client/smb2pdu.c:4629:
+ break;
+ } else
total: 0 errors, 1 warnings, 138 lines checked
On Tue, Jan 20, 2026 at 12:22 AM <nspmangalore@gmail.com> wrote:
>
> From: Shyam Prasad N <sprasad@microsoft.com>
>
> Today in most other code paths in cifs.ko, the decision of whether
> to retry a command depends on two mount options: retrans and hard.
> However, the read/write code paths diverged from this and would only
> retry if the error returned was -EAGAIN. However, there are other
> replayable errors in cifs.ko, for which is_replayable_errors helper
> was written. This change makes read/write codepaths consistent with
> other code-paths.
>
> This change also does the following:
> 1. The SMB2 read/write code diverged significantly (presumably since
> they were changed during netfs refactor at different times). This
> changes the response verification logic to be consistent.
> 2. Moves the netfs tracepoints to slightly different locations in order
> to make debugging easier.
>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
> fs/smb/client/cifsglob.h | 2 ++
> fs/smb/client/smb2pdu.c | 70 +++++++++++++++++++++++++++++++---------
> 2 files changed, 56 insertions(+), 16 deletions(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index 3eca5bfb70303..f6ebd3fd176d7 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -1507,6 +1507,8 @@ struct cifs_io_subrequest {
> int result;
> bool have_xid;
> bool replay;
> + unsigned int retries; /* number of retries so far */
> + unsigned int cur_sleep; /* time to sleep before replay */
> struct kvec iov[2];
> struct TCP_Server_Info *server;
> #ifdef CONFIG_CIFS_SMB_DIRECT
> diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> index 5d57c895ca37a..89f728392a734 100644
> --- a/fs/smb/client/smb2pdu.c
> +++ b/fs/smb/client/smb2pdu.c
> @@ -4616,17 +4616,19 @@ smb2_readv_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> case MID_RESPONSE_RECEIVED:
> credits.value = le16_to_cpu(shdr->CreditRequest);
> credits.instance = server->reconnect_instance;
> - /* result already set, check signature */
> - if (server->sign && !mid->decrypted) {
> - int rc;
> + rdata->result = smb2_check_receive(mid, server, 0);
> + if (rdata->result != 0) {
> + rdata->subreq.error = rdata->result;
> + if (is_replayable_error(rdata->result)) {
> + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_req_submitted);
> + __set_bit(NETFS_SREQ_NEED_RETRY, &rdata->subreq.flags);
> + } else {
> + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_bad);
> + }
> + break;
> + } else
> + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress);
>
> - iov_iter_truncate(&rqst.rq_iter, rdata->got_bytes);
> - rc = smb2_verify_signature(&rqst, server);
> - if (rc)
> - cifs_tcon_dbg(VFS, "SMB signature verification returned error = %d\n",
> - rc);
> - }
> - /* FIXME: should this be counted toward the initiating task? */
> task_io_account_read(rdata->got_bytes);
> cifs_stats_bytes_read(tcon, rdata->got_bytes);
> break;
> @@ -4748,7 +4750,7 @@ smb2_async_readv(struct cifs_io_subrequest *rdata)
> rc = smb2_new_read_req(
> (void **) &buf, &total_len, &io_parms, rdata, 0, 0);
> if (rc)
> - return rc;
> + goto out;
>
> if (smb3_encryption_required(io_parms.tcon))
> flags |= CIFS_TRANSFORM_REQ;
> @@ -4795,6 +4797,17 @@ smb2_async_readv(struct cifs_io_subrequest *rdata)
>
> async_readv_out:
> cifs_small_buf_release(buf);
> +
> +out:
> + /* if the send error is retryable, let netfs know about it */
> + if (is_replayable_error(rc) &&
> + smb2_should_replay(tcon,
> + &rdata->retries,
> + &rdata->cur_sleep)) {
> + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_retry_needed);
> + __set_bit(NETFS_SREQ_NEED_RETRY, &rdata->subreq.flags);
> + }
> +
> return rc;
> }
>
> @@ -4908,14 +4921,20 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
>
> switch (mid->mid_state) {
> case MID_RESPONSE_RECEIVED:
> - trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_progress);
> credits.value = le16_to_cpu(rsp->hdr.CreditRequest);
> credits.instance = server->reconnect_instance;
> result = smb2_check_receive(mid, server, 0);
> if (result != 0) {
> - trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_bad);
> + if (is_replayable_error(result)) {
> + trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_req_submitted);
> + __set_bit(NETFS_SREQ_NEED_RETRY, &wdata->subreq.flags);
> + } else {
> + wdata->subreq.error = result;
> + trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_bad);
> + }
> break;
> - }
> + } else
> + trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_progress);
>
> written = le32_to_cpu(rsp->DataLength);
> /*
> @@ -4930,7 +4949,7 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> cifs_stats_bytes_written(tcon, written);
>
> if (written < wdata->subreq.len) {
> - wdata->result = -ENOSPC;
> + result = -ENOSPC;
> } else if (written > 0) {
> wdata->subreq.len = written;
> __set_bit(NETFS_SREQ_MADE_PROGRESS, &wdata->subreq.flags);
> @@ -4972,6 +4991,7 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> }
> #endif
> if (result) {
> + wdata->result = result;
> cifs_stats_fail_inc(tcon, SMB2_WRITE_HE);
> trace_smb3_write_err(wdata->rreq->debug_id,
> wdata->subreq.debug_index,
> @@ -4994,6 +5014,14 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> server->credits, server->in_flight,
> 0, cifs_trace_rw_credits_write_response_clear);
> wdata->credits.value = 0;
> +
> + /* see if we need to retry */
> + if (is_replayable_error(wdata->result) &&
> + smb2_should_replay(tcon,
> + &wdata->retries,
> + &wdata->cur_sleep))
> + wdata->replay = true;
> +
> cifs_write_subrequest_terminated(wdata, result ?: written);
> release_mid(server, mid);
> trace_smb3_rw_credits(rreq_debug_id, subreq_debug_index, 0,
> @@ -5112,7 +5140,7 @@ smb2_async_writev(struct cifs_io_subrequest *wdata)
> }
> #endif
>
> - if (wdata->subreq.retry_count > 0)
> + if (wdata->replay)
> smb2_set_replay(server, &rqst);
>
> cifs_dbg(FYI, "async write at %llu %u bytes iter=%zx\n",
> @@ -5159,6 +5187,16 @@ smb2_async_writev(struct cifs_io_subrequest *wdata)
> async_writev_out:
> cifs_small_buf_release(req);
> out:
> + /* if the send error is retryable, let netfs know about it */
> + if (is_replayable_error(rc) &&
> + smb2_should_replay(tcon,
> + &wdata->retries,
> + &wdata->cur_sleep)) {
> + wdata->replay = true;
> + trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_retry_needed);
> + __set_bit(NETFS_SREQ_NEED_RETRY, &wdata->subreq.flags);
> + }
> +
> if (rc) {
> trace_smb3_rw_credits(wdata->rreq->debug_id,
> wdata->subreq.debug_index,
> --
> 2.43.0
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths
2026-01-20 17:03 ` David Howells
@ 2026-01-21 4:45 ` Shyam Prasad N
0 siblings, 0 replies; 18+ messages in thread
From: Shyam Prasad N @ 2026-01-21 4:45 UTC (permalink / raw)
To: David Howells; +Cc: linux-cifs, smfrench, pc, bharathsm, Shyam Prasad N
Thanks for the review, David.
On Tue, Jan 20, 2026 at 10:33 PM David Howells <dhowells@redhat.com> wrote:
>
> Hi Shyam,
>
> Some immediate thoughts for you.
>
> nspmangalore@gmail.com wrote:
>
> > + break;
> > + } else
> > + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress);
>
> The 'else' is superfluous, given the 'break'.
>
> > + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_retry_needed);
checkpatch warned me about this too. I felt that the same change
needed to go into both read and write codepath, one of which I had not
changed w.r.t the behaviour.
I don't mind submitting another version with this change.
>
> Btw, feel free to add more trace values if you want to distinguish 'retries'
> from 'replays' if that makes sense.
I think it should be fine.
In fact, I was wondering if we can move these trace points to within
the subreq_terminated functions in netfs itself. Do you see any
problem with that?
That can be a follow-up patch though.
>
> > break;
> > - }
> > + } else
> > + trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_progress);
>
> Superfluous 'else' again.
>
> > - wdata->result = -ENOSPC;
> > + result = -ENOSPC;
>
> I wonder if wdata->result is redundant. Can wdata->subreq.error be used
> instead (though that's unrelated to the subject of this patch).
I agree. I did notice that too. But wanted to make pointed changes
with this patch.
>
> David
>
--
Regards,
Shyam
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths
2026-01-21 1:56 ` Steve French
@ 2026-01-21 4:46 ` Shyam Prasad N
0 siblings, 0 replies; 18+ messages in thread
From: Shyam Prasad N @ 2026-01-21 4:46 UTC (permalink / raw)
To: Steve French; +Cc: linux-cifs, pc, bharathsm, dhowells, Shyam Prasad N
On Wed, Jan 21, 2026 at 7:26 AM Steve French <smfrench@gmail.com> wrote:
>
> Checkpatch flagged this with minor warning. Let me know if you do
> updated version of it
>
> 0004-cifs-make-retry-logic-in-read-write-path-consistent-.patch
> ------------------------------------------------------------------------
> WARNING: else is not generally useful after a break or return
> #64: FILE: fs/smb/client/smb2pdu.c:4629:
> + break;
> + } else
>
> total: 0 errors, 1 warnings, 138 lines checked
I saw that too Steve. I'll submit a v2 patch for this.
>
> On Tue, Jan 20, 2026 at 12:22 AM <nspmangalore@gmail.com> wrote:
> >
> > From: Shyam Prasad N <sprasad@microsoft.com>
> >
> > Today in most other code paths in cifs.ko, the decision of whether
> > to retry a command depends on two mount options: retrans and hard.
> > However, the read/write code paths diverged from this and would only
> > retry if the error returned was -EAGAIN. However, there are other
> > replayable errors in cifs.ko, for which is_replayable_errors helper
> > was written. This change makes read/write codepaths consistent with
> > other code-paths.
> >
> > This change also does the following:
> > 1. The SMB2 read/write code diverged significantly (presumably since
> > they were changed during netfs refactor at different times). This
> > changes the response verification logic to be consistent.
> > 2. Moves the netfs tracepoints to slightly different locations in order
> > to make debugging easier.
> >
> > Cc: David Howells <dhowells@redhat.com>
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> > fs/smb/client/cifsglob.h | 2 ++
> > fs/smb/client/smb2pdu.c | 70 +++++++++++++++++++++++++++++++---------
> > 2 files changed, 56 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> > index 3eca5bfb70303..f6ebd3fd176d7 100644
> > --- a/fs/smb/client/cifsglob.h
> > +++ b/fs/smb/client/cifsglob.h
> > @@ -1507,6 +1507,8 @@ struct cifs_io_subrequest {
> > int result;
> > bool have_xid;
> > bool replay;
> > + unsigned int retries; /* number of retries so far */
> > + unsigned int cur_sleep; /* time to sleep before replay */
> > struct kvec iov[2];
> > struct TCP_Server_Info *server;
> > #ifdef CONFIG_CIFS_SMB_DIRECT
> > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> > index 5d57c895ca37a..89f728392a734 100644
> > --- a/fs/smb/client/smb2pdu.c
> > +++ b/fs/smb/client/smb2pdu.c
> > @@ -4616,17 +4616,19 @@ smb2_readv_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> > case MID_RESPONSE_RECEIVED:
> > credits.value = le16_to_cpu(shdr->CreditRequest);
> > credits.instance = server->reconnect_instance;
> > - /* result already set, check signature */
> > - if (server->sign && !mid->decrypted) {
> > - int rc;
> > + rdata->result = smb2_check_receive(mid, server, 0);
> > + if (rdata->result != 0) {
> > + rdata->subreq.error = rdata->result;
> > + if (is_replayable_error(rdata->result)) {
> > + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_req_submitted);
> > + __set_bit(NETFS_SREQ_NEED_RETRY, &rdata->subreq.flags);
> > + } else {
> > + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_bad);
> > + }
> > + break;
> > + } else
> > + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress);
> >
> > - iov_iter_truncate(&rqst.rq_iter, rdata->got_bytes);
> > - rc = smb2_verify_signature(&rqst, server);
> > - if (rc)
> > - cifs_tcon_dbg(VFS, "SMB signature verification returned error = %d\n",
> > - rc);
> > - }
> > - /* FIXME: should this be counted toward the initiating task? */
> > task_io_account_read(rdata->got_bytes);
> > cifs_stats_bytes_read(tcon, rdata->got_bytes);
> > break;
> > @@ -4748,7 +4750,7 @@ smb2_async_readv(struct cifs_io_subrequest *rdata)
> > rc = smb2_new_read_req(
> > (void **) &buf, &total_len, &io_parms, rdata, 0, 0);
> > if (rc)
> > - return rc;
> > + goto out;
> >
> > if (smb3_encryption_required(io_parms.tcon))
> > flags |= CIFS_TRANSFORM_REQ;
> > @@ -4795,6 +4797,17 @@ smb2_async_readv(struct cifs_io_subrequest *rdata)
> >
> > async_readv_out:
> > cifs_small_buf_release(buf);
> > +
> > +out:
> > + /* if the send error is retryable, let netfs know about it */
> > + if (is_replayable_error(rc) &&
> > + smb2_should_replay(tcon,
> > + &rdata->retries,
> > + &rdata->cur_sleep)) {
> > + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_retry_needed);
> > + __set_bit(NETFS_SREQ_NEED_RETRY, &rdata->subreq.flags);
> > + }
> > +
> > return rc;
> > }
> >
> > @@ -4908,14 +4921,20 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> >
> > switch (mid->mid_state) {
> > case MID_RESPONSE_RECEIVED:
> > - trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_progress);
> > credits.value = le16_to_cpu(rsp->hdr.CreditRequest);
> > credits.instance = server->reconnect_instance;
> > result = smb2_check_receive(mid, server, 0);
> > if (result != 0) {
> > - trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_bad);
> > + if (is_replayable_error(result)) {
> > + trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_req_submitted);
> > + __set_bit(NETFS_SREQ_NEED_RETRY, &wdata->subreq.flags);
> > + } else {
> > + wdata->subreq.error = result;
> > + trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_bad);
> > + }
> > break;
> > - }
> > + } else
> > + trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_progress);
> >
> > written = le32_to_cpu(rsp->DataLength);
> > /*
> > @@ -4930,7 +4949,7 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> > cifs_stats_bytes_written(tcon, written);
> >
> > if (written < wdata->subreq.len) {
> > - wdata->result = -ENOSPC;
> > + result = -ENOSPC;
> > } else if (written > 0) {
> > wdata->subreq.len = written;
> > __set_bit(NETFS_SREQ_MADE_PROGRESS, &wdata->subreq.flags);
> > @@ -4972,6 +4991,7 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> > }
> > #endif
> > if (result) {
> > + wdata->result = result;
> > cifs_stats_fail_inc(tcon, SMB2_WRITE_HE);
> > trace_smb3_write_err(wdata->rreq->debug_id,
> > wdata->subreq.debug_index,
> > @@ -4994,6 +5014,14 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> > server->credits, server->in_flight,
> > 0, cifs_trace_rw_credits_write_response_clear);
> > wdata->credits.value = 0;
> > +
> > + /* see if we need to retry */
> > + if (is_replayable_error(wdata->result) &&
> > + smb2_should_replay(tcon,
> > + &wdata->retries,
> > + &wdata->cur_sleep))
> > + wdata->replay = true;
> > +
> > cifs_write_subrequest_terminated(wdata, result ?: written);
> > release_mid(server, mid);
> > trace_smb3_rw_credits(rreq_debug_id, subreq_debug_index, 0,
> > @@ -5112,7 +5140,7 @@ smb2_async_writev(struct cifs_io_subrequest *wdata)
> > }
> > #endif
> >
> > - if (wdata->subreq.retry_count > 0)
> > + if (wdata->replay)
> > smb2_set_replay(server, &rqst);
> >
> > cifs_dbg(FYI, "async write at %llu %u bytes iter=%zx\n",
> > @@ -5159,6 +5187,16 @@ smb2_async_writev(struct cifs_io_subrequest *wdata)
> > async_writev_out:
> > cifs_small_buf_release(req);
> > out:
> > + /* if the send error is retryable, let netfs know about it */
> > + if (is_replayable_error(rc) &&
> > + smb2_should_replay(tcon,
> > + &wdata->retries,
> > + &wdata->cur_sleep)) {
> > + wdata->replay = true;
> > + trace_netfs_sreq(&wdata->subreq, netfs_sreq_trace_io_retry_needed);
> > + __set_bit(NETFS_SREQ_NEED_RETRY, &wdata->subreq.flags);
> > + }
> > +
> > if (rc) {
> > trace_smb3_rw_credits(wdata->rreq->debug_id,
> > wdata->subreq.debug_index,
> > --
> > 2.43.0
> >
>
>
> --
> Thanks,
>
> Steve
--
Regards,
Shyam
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error
2026-01-20 6:21 [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error nspmangalore
` (2 preceding siblings ...)
2026-01-20 6:21 ` [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths nspmangalore
@ 2026-01-21 23:08 ` David Howells
2026-01-21 23:18 ` David Howells
4 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2026-01-21 23:08 UTC (permalink / raw)
To: nspmangalore
Cc: dhowells, linux-cifs, smfrench, pc, bharathsm, Shyam Prasad N
Btw, can you cc netfs@lists.linux.dev? (see the MAINTAINERS file)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error
2026-01-20 6:21 [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error nspmangalore
` (3 preceding siblings ...)
2026-01-21 23:08 ` [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error David Howells
@ 2026-01-21 23:18 ` David Howells
2026-01-29 12:13 ` Shyam Prasad N
4 siblings, 1 reply; 18+ messages in thread
From: David Howells @ 2026-01-21 23:18 UTC (permalink / raw)
To: nspmangalore
Cc: dhowells, linux-cifs, smfrench, pc, bharathsm, Shyam Prasad N
nspmangalore@gmail.com wrote:
> @@ -547,13 +547,15 @@ void netfs_read_subreq_terminated(struct netfs_io_subrequest *subreq)
> }
>
> if (unlikely(subreq->error < 0)) {
> - trace_netfs_failure(rreq, subreq, subreq->error, netfs_fail_read);
> if (subreq->source == NETFS_READ_FROM_CACHE) {
> netfs_stat(&netfs_n_rh_read_failed);
> __set_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
> } else {
> netfs_stat(&netfs_n_rh_download_failed);
> - __set_bit(NETFS_SREQ_FAILED, &subreq->flags);
> + if (!test_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags)) {
> + __set_bit(NETFS_SREQ_FAILED, &subreq->flags);
> + trace_netfs_failure(rreq, subreq, subreq->error, netfs_fail_read);
> + }
> }
> trace_netfs_rreq(rreq, netfs_rreq_trace_set_pause);
> set_bit(NETFS_RREQ_PAUSE, &rreq->flags);
I think I suggested moving the check for NETFS_SREQ_NEED_RETRY up in the
function - above any checks of subreq->error, but after the initial stat
counting.
Ditto for netfs_write_subrequest_terminated(). Actually, the
transferred_or_error argument of that should be got rid of and the filesystem
update the subreq->error and subreq->transferred fields directly as for reads.
I can poke at this tomorrow if you want.
David
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] netfs: avoid double increment of retry_count in subreq
2026-01-20 6:21 ` [PATCH 2/4] netfs: avoid double increment of retry_count in subreq nspmangalore
@ 2026-01-21 23:23 ` David Howells
0 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2026-01-21 23:23 UTC (permalink / raw)
To: nspmangalore
Cc: dhowells, linux-cifs, smfrench, pc, bharathsm, Shyam Prasad N
nspmangalore@gmail.com wrote:
> From: Shyam Prasad N <sprasad@microsoft.com>
>
> This change fixes the instance of double incrementing of
> retry_count. The increment of this count already happens
> when netfs_reissue_write gets called. Incrementing this
> value before is not necessary.
>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
Fixes: 4acb665cf4f3 ("netfs: Work around recursion by abandoning retry if nothing read")
Acked-by: David Howells <dhowells@redhat.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] cifs: Initialize cur_sleep value if not already done
2026-01-20 6:21 ` [PATCH 3/4] cifs: Initialize cur_sleep value if not already done nspmangalore
@ 2026-01-21 23:28 ` David Howells
0 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2026-01-21 23:28 UTC (permalink / raw)
To: nspmangalore
Cc: dhowells, linux-cifs, smfrench, pc, bharathsm, Shyam Prasad N
This doesn't affect smb2_async_readv() and smb2_async_writev(). Should
netfslib do some sort of backoff on retrying?
David
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths
2026-01-20 6:21 ` [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths nspmangalore
` (2 preceding siblings ...)
2026-01-21 1:56 ` Steve French
@ 2026-01-21 23:36 ` David Howells
2026-01-21 23:50 ` David Howells
3 siblings, 1 reply; 18+ messages in thread
From: David Howells @ 2026-01-21 23:36 UTC (permalink / raw)
To: nspmangalore
Cc: dhowells, linux-cifs, smfrench, pc, bharathsm, Shyam Prasad N
nspmangalore@gmail.com wrote:
> @@ -4994,6 +5014,14 @@ smb2_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> server->credits, server->in_flight,
> 0, cifs_trace_rw_credits_write_response_clear);
> wdata->credits.value = 0;
> +
> + /* see if we need to retry */
> + if (is_replayable_error(wdata->result) &&
> + smb2_should_replay(tcon,
> + &wdata->retries,
> + &wdata->cur_sleep))
> + wdata->replay = true;
> +
This is really, really going to suck performance wise. ->callback() is called
in the context of the I/O thread for that socket. smb2_should_replay() does
an msleep() of up to 2 seconds. That means you aren't going to be processing
*any* messages coming down the socket from the server for the duration.
Better to offload the pause to netfslib if we can.
David
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths
2026-01-21 23:36 ` David Howells
@ 2026-01-21 23:50 ` David Howells
2026-01-29 11:38 ` Shyam Prasad N
0 siblings, 1 reply; 18+ messages in thread
From: David Howells @ 2026-01-21 23:50 UTC (permalink / raw)
To: nspmangalore
Cc: dhowells, linux-cifs, smfrench, pc, bharathsm, Shyam Prasad N
David Howells <dhowells@redhat.com> wrote:
> Better to offload the pause to netfslib if we can.
I would suggest you look at doing it in netfs_retry_writes(). Something like:
(1) Add a timestamp to netfs_io_request to record either the first op being
issued or the last op being issued.
(2) After netfs_retry_writes() finishes waiting for subreqs to quiesce, wait
for the requisite amount of time since the timestamp recorded in (1)
before continuing the retry.
(3) Add a method to netfs_request_ops to allow netfslib to ask the filesystem
what backoff delay it wants to insert. This could call
smb2_should_replay().
Alternatively, set a flag on cifs_io_request indicating backoff is required
and do it in cifs_prepare_write() before waiting for credits if the flag is
set - or maybe in cifs_issue_write() - at which point clear the flag.
David
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths
2026-01-21 23:50 ` David Howells
@ 2026-01-29 11:38 ` Shyam Prasad N
0 siblings, 0 replies; 18+ messages in thread
From: Shyam Prasad N @ 2026-01-29 11:38 UTC (permalink / raw)
To: David Howells; +Cc: linux-cifs, smfrench, pc, bharathsm, Shyam Prasad N
On Thu, Jan 22, 2026 at 5:20 AM David Howells <dhowells@redhat.com> wrote:
>
> David Howells <dhowells@redhat.com> wrote:
>
> > Better to offload the pause to netfslib if we can.
>
> I would suggest you look at doing it in netfs_retry_writes(). Something like:
>
> (1) Add a timestamp to netfs_io_request to record either the first op being
> issued or the last op being issued.
>
> (2) After netfs_retry_writes() finishes waiting for subreqs to quiesce, wait
> for the requisite amount of time since the timestamp recorded in (1)
> before continuing the retry.
>
> (3) Add a method to netfs_request_ops to allow netfslib to ask the filesystem
> what backoff delay it wants to insert. This could call
> smb2_should_replay().
>
> Alternatively, set a flag on cifs_io_request indicating backoff is required
> and do it in cifs_prepare_write() before waiting for credits if the flag is
> set - or maybe in cifs_issue_write() - at which point clear the flag.
>
> David
>
I get your point here David. (And good catch!)
The problem in my patch was the sleep in the cifsd thread.
I think I can handle the sleep fully in cifs.ko by changing where the
msleep happens.
I plan to do the following:
1. Take the msleep out of smb2_should_replay
2. Sleep before doing the actual replay (This will be the back-off
*before* replaying)
That way, I'll be able to safely call smb2_should_replay in the
callback functions and sleep in smb2_async_readv/writev functions.
I'll submit a revised patch series soon.
--
Regards,
Shyam
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error
2026-01-21 23:18 ` David Howells
@ 2026-01-29 12:13 ` Shyam Prasad N
2026-01-29 13:43 ` David Howells
0 siblings, 1 reply; 18+ messages in thread
From: Shyam Prasad N @ 2026-01-29 12:13 UTC (permalink / raw)
To: David Howells; +Cc: linux-cifs, smfrench, pc, bharathsm, Shyam Prasad N
On Thu, Jan 22, 2026 at 4:48 AM David Howells <dhowells@redhat.com> wrote:
>
> nspmangalore@gmail.com wrote:
>
> > @@ -547,13 +547,15 @@ void netfs_read_subreq_terminated(struct netfs_io_subrequest *subreq)
> > }
> >
> > if (unlikely(subreq->error < 0)) {
> > - trace_netfs_failure(rreq, subreq, subreq->error, netfs_fail_read);
> > if (subreq->source == NETFS_READ_FROM_CACHE) {
> > netfs_stat(&netfs_n_rh_read_failed);
> > __set_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
> > } else {
> > netfs_stat(&netfs_n_rh_download_failed);
> > - __set_bit(NETFS_SREQ_FAILED, &subreq->flags);
> > + if (!test_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags)) {
> > + __set_bit(NETFS_SREQ_FAILED, &subreq->flags);
> > + trace_netfs_failure(rreq, subreq, subreq->error, netfs_fail_read);
> > + }
> > }
> > trace_netfs_rreq(rreq, netfs_rreq_trace_set_pause);
> > set_bit(NETFS_RREQ_PAUSE, &rreq->flags);
>
> I think I suggested moving the check for NETFS_SREQ_NEED_RETRY up in the
> function - above any checks of subreq->error, but after the initial stat
> counting.
So you want to do this check regardless of whether there's an error or not?
In that case, I think I'll still need to check if there is an error to
set NETFS_RREQ_PAUSE only on error, right?
>
> Ditto for netfs_write_subrequest_terminated(). Actually, the
> transferred_or_error argument of that should be got rid of and the filesystem
> update the subreq->error and subreq->transferred fields directly as for reads.
>
I can try making the write path similar to read. Passing just the
subreq. But that will mean changes to all the callers.
I hope that's okay.
> I can poke at this tomorrow if you want.
>
> David
>
--
Regards,
Shyam
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error
2026-01-29 12:13 ` Shyam Prasad N
@ 2026-01-29 13:43 ` David Howells
0 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2026-01-29 13:43 UTC (permalink / raw)
To: Shyam Prasad N
Cc: dhowells, linux-cifs, smfrench, pc, bharathsm, Shyam Prasad N
Shyam Prasad N <nspmangalore@gmail.com> wrote:
> > I think I suggested moving the check for NETFS_SREQ_NEED_RETRY up in the
> > function - above any checks of subreq->error, but after the initial stat
> > counting.
>
> So you want to do this check regardless of whether there's an error or not?
Yes. There's likely only to be a need to retry if there's been an error
(though there is the possibility of the filesystem returning a short read,
say, and needing a retry to complete it). In any case, we can make the
setting of this flag up to the filesystem: does it think that a retry is
warranted?
> In that case, I think I'll still need to check if there is an error to
> set NETFS_RREQ_PAUSE only on error, right?
It needs to be set if a retry is requested as the retry thread will wait for
outstanding ops to quiesce - and if more are still being generated, that's
potentially a problem. And for AFS, for example, the need to do a retry might
mean that we need to switch server - so we probably do want to throw a hold on
further subrequest generation until we've probed the problem.
David
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2026-01-29 13:43 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-20 6:21 [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error nspmangalore
2026-01-20 6:21 ` [PATCH 2/4] netfs: avoid double increment of retry_count in subreq nspmangalore
2026-01-21 23:23 ` David Howells
2026-01-20 6:21 ` [PATCH 3/4] cifs: Initialize cur_sleep value if not already done nspmangalore
2026-01-21 23:28 ` David Howells
2026-01-20 6:21 ` [PATCH 4/4] cifs: make retry logic in read/write path consistent with other paths nspmangalore
2026-01-20 11:32 ` kernel test robot
2026-01-20 17:03 ` David Howells
2026-01-21 4:45 ` Shyam Prasad N
2026-01-21 1:56 ` Steve French
2026-01-21 4:46 ` Shyam Prasad N
2026-01-21 23:36 ` David Howells
2026-01-21 23:50 ` David Howells
2026-01-29 11:38 ` Shyam Prasad N
2026-01-21 23:08 ` [PATCH 1/4] netfs: when subreq is marked for retry, do not check if it faced an error David Howells
2026-01-21 23:18 ` David Howells
2026-01-29 12:13 ` Shyam Prasad N
2026-01-29 13:43 ` David Howells
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox