Shyam patch review #118
Open
Shyam patch review #118
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rror 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 <[email protected]> Signed-off-by: Shyam Prasad N <[email protected]> Signed-off-by: Steve French <[email protected]>
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 <[email protected]> Signed-off-by: Shyam Prasad N <[email protected]> Signed-off-by: Steve French <[email protected]>
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 <[email protected]> Signed-off-by: Steve French <[email protected]>
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 <[email protected]> Signed-off-by: Shyam Prasad N <[email protected]> Signed-off-by: Steve French <[email protected]>
There was a problem hiding this comment.
Pull request overview
This pull request updates the SMB client and netfs subsystems to improve error handling and retry logic for read/write operations. The changes focus on properly managing replayable errors and ensuring consistent state management across retry attempts.
Changes:
- Enhanced error handling in SMB2 read/write callbacks to properly detect and flag replayable errors
- Added retry tracking fields (retries, cur_sleep) to cifs_io_subrequest structure
- Improved error state management in netfs retry logic by clearing errors on reissue
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| fs/smb/client/smb2pdu.c | Updated read/write callbacks and async functions to handle replayable errors and set retry flags |
| fs/smb/client/smb2ops.c | Added initialization of cur_sleep to prevent uninitialized value usage |
| fs/smb/client/cifsglob.h | Added retries and cur_sleep fields to cifs_io_subrequest for retry tracking |
| fs/netfs/write_retry.c | Removed retry_count increment (moved to write_issue.c) |
| fs/netfs/write_issue.c | Added error clearing on reissue to prevent stale error propagation |
| fs/netfs/write_collect.c | Modified error handling to skip failure tracking when retry is flagged |
| fs/netfs/read_retry.c | Added error clearing on read reissue and simplified abandon logic |
| fs/netfs/read_collect.c | Modified to avoid marking failed or tracing when retry is already flagged |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
4 client changesets from Shyam