From: David Howells <dhowells@redhat.com> To: Christian Brauner <christian@brauner.io> Cc: David Howells <dhowells@redhat.com>, Jeff Layton <jlayton@kernel.org>, Matthew Wilcox <willy@infradead.org>, netfs@lists.linux.dev, linux-afs@lists.infradead.org, linux-cifs@vger.kernel.org, linux-nfs@vger.kernel.org, ceph-devel@vger.kernel.org, v9fs@lists.linux.dev, linux-erofs@lists.ozlabs.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Jeffrey Altman <jaltman@auristor.com>, Marc Dionne <marc.dionne@auristor.com> Subject: [PATCH 08/10] afs: Fix error handling with lookup via FS.InlineBulkStatus Date: Mon, 22 Jan 2024 12:38:41 +0000 [thread overview] Message-ID: <20240122123845.3822570-9-dhowells@redhat.com> (raw) In-Reply-To: <20240122123845.3822570-1-dhowells@redhat.com> When afs does a lookup, it tries to use FS.InlineBulkStatus to preemptively look up a bunch of files in the parent directory and cache this locally, on the basis that we might want to look at them too (for example if someone does an ls on a directory, they may want want to then stat every file listed). FS.InlineBulkStatus can be considered a compound op with the normal abort code applying to the compound as a whole. Each status fetch within the compound is then given its own individual abort code - but assuming no error that prevents the bulk fetch from returning the compound result will be 0, even if all the constituent status fetches failed. At the conclusion of afs_do_lookup(), we should use the abort code from the appropriate status to determine the error to return, if any - but instead it is assumed that we were successful if the op as a whole succeeded and we return an incompletely initialised inode, resulting in ENOENT, no matter the actual reason. In the particular instance reported, a vnode with no permission granted to be accessed is being given a UAEACCES abort code which should be reported as EACCES, but is instead being reported as ENOENT. Fix this by abandoning the inode (which will be cleaned up with the op) if file[1] has an abort code indicated and turn that abort code into an error instead. Whilst we're at it, add a tracepoint so that the abort codes of the individual subrequests of FS.InlineBulkStatus can be logged. At the moment only the container abort code can be 0. Fixes: e49c7b2f6de7 ("afs: Build an abstraction around an "operation" concept") Reported-by: Jeffrey Altman <jaltman@auristor.com> Signed-off-by: David Howells <dhowells@redhat.com> Reviewed-by: Marc Dionne <marc.dionne@auristor.com> cc: linux-afs@lists.infradead.org --- fs/afs/dir.c | 12 +++++++++--- include/trace/events/afs.h | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/fs/afs/dir.c b/fs/afs/dir.c index eface67ccc06..b5b8de521f99 100644 --- a/fs/afs/dir.c +++ b/fs/afs/dir.c @@ -716,6 +716,8 @@ static void afs_do_lookup_success(struct afs_operation *op) break; } + if (vp->scb.status.abort_code) + trace_afs_bulkstat_error(op, &vp->fid, i, vp->scb.status.abort_code); if (!vp->scb.have_status && !vp->scb.have_error) continue; @@ -905,12 +907,16 @@ static struct inode *afs_do_lookup(struct inode *dir, struct dentry *dentry, afs_begin_vnode_operation(op); afs_wait_for_operation(op); } - inode = ERR_PTR(afs_op_error(op)); out_op: if (!afs_op_error(op)) { - inode = &op->file[1].vnode->netfs.inode; - op->file[1].vnode = NULL; + if (op->file[1].scb.status.abort_code) { + afs_op_accumulate_error(op, -ECONNABORTED, + op->file[1].scb.status.abort_code); + } else { + inode = &op->file[1].vnode->netfs.inode; + op->file[1].vnode = NULL; + } } if (op->file[0].scb.have_status) diff --git a/include/trace/events/afs.h b/include/trace/events/afs.h index 8d73171cb9f0..08f2c93d6b16 100644 --- a/include/trace/events/afs.h +++ b/include/trace/events/afs.h @@ -1071,6 +1071,31 @@ TRACE_EVENT(afs_file_error, __print_symbolic(__entry->where, afs_file_errors)) ); +TRACE_EVENT(afs_bulkstat_error, + TP_PROTO(struct afs_operation *op, struct afs_fid *fid, unsigned int index, s32 abort), + + TP_ARGS(op, fid, index, abort), + + TP_STRUCT__entry( + __field_struct(struct afs_fid, fid) + __field(unsigned int, op) + __field(unsigned int, index) + __field(s32, abort) + ), + + TP_fast_assign( + __entry->op = op->debug_id; + __entry->fid = *fid; + __entry->index = index; + __entry->abort = abort; + ), + + TP_printk("OP=%08x[%02x] %llx:%llx:%x a=%d", + __entry->op, __entry->index, + __entry->fid.vid, __entry->fid.vnode, __entry->fid.unique, + __entry->abort) + ); + TRACE_EVENT(afs_cm_no_server, TP_PROTO(struct afs_call *call, struct sockaddr_rxrpc *srx),
WARNING: multiple messages have this Message-ID (diff)
From: David Howells <dhowells@redhat.com> To: Christian Brauner <christian@brauner.io> Cc: linux-cifs@vger.kernel.org, linux-nfs@vger.kernel.org, Jeffrey Altman <jaltman@auristor.com>, v9fs@lists.linux.dev, Jeff Layton <jlayton@kernel.org>, linux-kernel@vger.kernel.org, Matthew Wilcox <willy@infradead.org>, David Howells <dhowells@redhat.com>, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, netfs@lists.linux.dev, ceph-devel@vger.kernel.org, linux-erofs@lists.ozlabs.org, linux-afs@lists.infradead.org, Marc Dionne <marc.dionne@auristor.com> Subject: [PATCH 08/10] afs: Fix error handling with lookup via FS.InlineBulkStatus Date: Mon, 22 Jan 2024 12:38:41 +0000 [thread overview] Message-ID: <20240122123845.3822570-9-dhowells@redhat.com> (raw) In-Reply-To: <20240122123845.3822570-1-dhowells@redhat.com> When afs does a lookup, it tries to use FS.InlineBulkStatus to preemptively look up a bunch of files in the parent directory and cache this locally, on the basis that we might want to look at them too (for example if someone does an ls on a directory, they may want want to then stat every file listed). FS.InlineBulkStatus can be considered a compound op with the normal abort code applying to the compound as a whole. Each status fetch within the compound is then given its own individual abort code - but assuming no error that prevents the bulk fetch from returning the compound result will be 0, even if all the constituent status fetches failed. At the conclusion of afs_do_lookup(), we should use the abort code from the appropriate status to determine the error to return, if any - but instead it is assumed that we were successful if the op as a whole succeeded and we return an incompletely initialised inode, resulting in ENOENT, no matter the actual reason. In the particular instance reported, a vnode with no permission granted to be accessed is being given a UAEACCES abort code which should be reported as EACCES, but is instead being reported as ENOENT. Fix this by abandoning the inode (which will be cleaned up with the op) if file[1] has an abort code indicated and turn that abort code into an error instead. Whilst we're at it, add a tracepoint so that the abort codes of the individual subrequests of FS.InlineBulkStatus can be logged. At the moment only the container abort code can be 0. Fixes: e49c7b2f6de7 ("afs: Build an abstraction around an "operation" concept") Reported-by: Jeffrey Altman <jaltman@auristor.com> Signed-off-by: David Howells <dhowells@redhat.com> Reviewed-by: Marc Dionne <marc.dionne@auristor.com> cc: linux-afs@lists.infradead.org --- fs/afs/dir.c | 12 +++++++++--- include/trace/events/afs.h | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/fs/afs/dir.c b/fs/afs/dir.c index eface67ccc06..b5b8de521f99 100644 --- a/fs/afs/dir.c +++ b/fs/afs/dir.c @@ -716,6 +716,8 @@ static void afs_do_lookup_success(struct afs_operation *op) break; } + if (vp->scb.status.abort_code) + trace_afs_bulkstat_error(op, &vp->fid, i, vp->scb.status.abort_code); if (!vp->scb.have_status && !vp->scb.have_error) continue; @@ -905,12 +907,16 @@ static struct inode *afs_do_lookup(struct inode *dir, struct dentry *dentry, afs_begin_vnode_operation(op); afs_wait_for_operation(op); } - inode = ERR_PTR(afs_op_error(op)); out_op: if (!afs_op_error(op)) { - inode = &op->file[1].vnode->netfs.inode; - op->file[1].vnode = NULL; + if (op->file[1].scb.status.abort_code) { + afs_op_accumulate_error(op, -ECONNABORTED, + op->file[1].scb.status.abort_code); + } else { + inode = &op->file[1].vnode->netfs.inode; + op->file[1].vnode = NULL; + } } if (op->file[0].scb.have_status) diff --git a/include/trace/events/afs.h b/include/trace/events/afs.h index 8d73171cb9f0..08f2c93d6b16 100644 --- a/include/trace/events/afs.h +++ b/include/trace/events/afs.h @@ -1071,6 +1071,31 @@ TRACE_EVENT(afs_file_error, __print_symbolic(__entry->where, afs_file_errors)) ); +TRACE_EVENT(afs_bulkstat_error, + TP_PROTO(struct afs_operation *op, struct afs_fid *fid, unsigned int index, s32 abort), + + TP_ARGS(op, fid, index, abort), + + TP_STRUCT__entry( + __field_struct(struct afs_fid, fid) + __field(unsigned int, op) + __field(unsigned int, index) + __field(s32, abort) + ), + + TP_fast_assign( + __entry->op = op->debug_id; + __entry->fid = *fid; + __entry->index = index; + __entry->abort = abort; + ), + + TP_printk("OP=%08x[%02x] %llx:%llx:%x a=%d", + __entry->op, __entry->index, + __entry->fid.vid, __entry->fid.vnode, __entry->fid.unique, + __entry->abort) + ); + TRACE_EVENT(afs_cm_no_server, TP_PROTO(struct afs_call *call, struct sockaddr_rxrpc *srx),
next prev parent reply other threads:[~2024-01-22 12:39 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-01-22 12:38 [PATCH 00/10] netfs, afs, cifs, cachefiles, erofs: Miscellaneous fixes David Howells 2024-01-22 12:38 ` David Howells 2024-01-22 12:38 ` [PATCH 01/10] netfs: Don't use certain internal folio_*() functions David Howells 2024-01-22 12:38 ` David Howells 2024-01-22 15:38 ` Jeff Layton 2024-01-22 15:38 ` Jeff Layton 2024-01-22 16:10 ` Matthew Wilcox 2024-01-22 16:10 ` Matthew Wilcox 2024-01-22 17:22 ` David Howells 2024-01-22 17:22 ` David Howells 2024-01-22 12:38 ` [PATCH 02/10] afs: " David Howells 2024-01-22 12:38 ` David Howells 2024-01-22 12:38 ` [PATCH 03/10] cifs: " David Howells 2024-01-22 12:38 ` David Howells 2024-01-22 12:38 ` [PATCH 04/10] netfs, fscache: Prevent Oops in fscache_put_cache() David Howells 2024-01-22 12:38 ` David Howells 2024-01-22 12:38 ` [PATCH 05/10] netfs: Fix a NULL vs IS_ERR() check in netfs_perform_write() David Howells 2024-01-22 12:38 ` David Howells 2024-01-22 12:38 ` [PATCH 06/10] cachefiles, erofs: Fix NULL deref in when cachefiles is not doing ondemand-mode David Howells 2024-01-22 12:38 ` David Howells 2024-01-22 13:48 ` Jingbo Xu 2024-01-22 13:48 ` Jingbo Xu 2024-01-22 15:27 ` Gao Xiang 2024-01-22 15:27 ` Gao Xiang 2024-01-22 22:01 ` David Howells 2024-01-22 22:01 ` David Howells 2024-01-22 12:38 ` [PATCH 07/10] afs: Hide silly-rename files from userspace David Howells 2024-01-22 12:38 ` David Howells 2024-01-22 12:38 ` David Howells [this message] 2024-01-22 12:38 ` [PATCH 08/10] afs: Fix error handling with lookup via FS.InlineBulkStatus David Howells 2024-01-22 12:38 ` [PATCH 09/10] afs: Remove afs_dynroot_d_revalidate() as it is redundant David Howells 2024-01-22 12:38 ` David Howells 2024-01-22 12:38 ` [PATCH 10/10] afs: Fix missing/incorrect unlocking of RCU read lock David Howells 2024-01-22 12:38 ` David Howells 2024-01-22 15:18 ` [PATCH 00/10] netfs, afs, cifs, cachefiles, erofs: Miscellaneous fixes Christian Brauner 2024-01-22 15:18 ` Christian Brauner 2024-01-23 15:03 ` Christian Brauner 2024-01-23 15:03 ` Christian Brauner 2024-01-22 15:57 ` Jeff Layton 2024-01-22 15:57 ` Jeff Layton
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20240122123845.3822570-9-dhowells@redhat.com \ --to=dhowells@redhat.com \ --cc=ceph-devel@vger.kernel.org \ --cc=christian@brauner.io \ --cc=jaltman@auristor.com \ --cc=jlayton@kernel.org \ --cc=linux-afs@lists.infradead.org \ --cc=linux-cifs@vger.kernel.org \ --cc=linux-erofs@lists.ozlabs.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linux-nfs@vger.kernel.org \ --cc=marc.dionne@auristor.com \ --cc=netfs@lists.linux.dev \ --cc=v9fs@lists.linux.dev \ --cc=willy@infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.