On Tue, 2024-03-19 at 11:36 +1100, NeilBrown wrote:
>
> I've been reviewing some nfs/direct.c patches for possible backport
> to
> one of our older enterprise kernels and I've found something that
> looks
> wrong.
>
> It isn't clear to me how to exercise the code so I haven't be able to
> trigger a problem. I'm hoping that someone could either explain when
> this code runs, or confirm if the code is correct or not.
>
> Commit 954998b60caa ("NFS: Fix error handling for O_DIRECT write
> scheduling")
>
> adds an extra call to nfs_release_request() but I cannot find any
> place
> that an extra reference is taken.
>
> The code currently reads:
>
> while (!list_empty(&reqs)) {
> req = nfs_list_entry(reqs.next);
> nfs_list_remove_request(req);
> nfs_unlock_and_release_request(req);
> if (desc.pg_error == -EAGAIN) {
> nfs_mark_request_commit(req, NULL, &cinfo,
> 0);
> } else {
> spin_lock(&dreq->lock);
> nfs_direct_truncate_request(dreq, req);
> spin_unlock(&dreq->lock);
> nfs_release_request(req);
> }
> }
>
> after the nfs_unlock_and_release_request() call I would expect that
> the
> request could be freed, so that nfs_mark_request_commit() or the
> nfs_release_request() could cause a problem.
>
> Superficially it looks like the call should be simply
> nfs_unlock_request(). This would follow the
> list_remove;unlock;mark_commit pattern also found in
> nfs_direct_write_reschedule_io().
>
> Do we need:
> --- a/fs/nfs/direct.c
> +++ b/fs/nfs/direct.c
> @@ -581,7 +581,7 @@ static void nfs_direct_write_reschedule(struct
> nfs_direct_req *dreq)
> while (!list_empty(&reqs)) {
> req = nfs_list_entry(reqs.next);
> nfs_list_remove_request(req);
> - nfs_unlock_and_release_request(req);
> + nfs_unlock_request(req);
> if (desc.pg_error == -EAGAIN) {
> nfs_mark_request_commit(req, NULL, &cinfo,
> 0);
> } else {
See the full code that was changed:
- list_for_each_entry_safe(req, tmp, &reqs, wb_list) {
+ while (!list_empty(&reqs)) {
+ req = nfs_list_entry(reqs.next);
/* Bump the transmission count */
req->wb_nio++;
if (!nfs_pageio_add_request(&desc, req)) {
- nfs_list_move_request(req, &failed);
spin_lock(&cinfo.inode->i_lock);
- dreq->flags = 0;
- if (desc.pg_error < 0)
+ if (dreq->error < 0) {
+ desc.pg_error = dreq->error;
+ } else if (desc.pg_error != -EAGAIN) {
+ dreq->flags = 0;
+ if (!desc.pg_error)
+ desc.pg_error = -EIO;
dreq->error = desc.pg_error;
- else
- dreq->error = -EIO;
+ } else
+ dreq->flags =
NFS_ODIRECT_RESCHED_WRITES;
spin_unlock(&cinfo.inode->i_lock);
+ break;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Prior to this patch, we did not break out of the loop until the entire
"reqs" list hand been handled.
}
nfs_release_request(req);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Prior to this patch, every request on the "reqs" list was released,
whether or not they were being moved to the "failed" list.
}
nfs_pageio_complete(&desc);
- while (!list_empty(&failed)) {
- req = nfs_list_entry(failed.next);
+ while (!list_empty(&reqs)) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Prior to this patch, every request that was being handled here had
already seen a call to nfs_release_request() because we had already
gone through the entire list of "reqs".
With this patch applied, we're now handling all the requests that are
left on "reqs", and that have not been released.
+ req = nfs_list_entry(reqs.next);
nfs_list_remove_request(req);
nfs_unlock_and_release_request(req);
+ if (desc.pg_error == -EAGAIN)
+ nfs_mark_request_commit(req, NULL, &cinfo, 0);
+ else
+ nfs_release_request(req);
}
>
> ??
>
> Thanks,
> NeilBrown
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
