Message ID | 140431.1704208899@warthog.procyon.org.uk |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-14555-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:6f82:b0:100:9c79:88ff with SMTP id tb2csp4508664dyb; Tue, 2 Jan 2024 07:22:12 -0800 (PST) X-Google-Smtp-Source: AGHT+IH53YUGAj5W4Xlo+O3n1UtFU0l3pcRbvXs8uO/Fe+jW3gvJQ6xVDp6SiIz5Vx3oXReKkRxQ X-Received: by 2002:a05:620a:1a85:b0:781:5c66:c71d with SMTP id bl5-20020a05620a1a8500b007815c66c71dmr17073244qkb.45.1704208931997; Tue, 02 Jan 2024 07:22:11 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1704208931; cv=none; d=google.com; s=arc-20160816; b=CgGxz5jKCTtpI8HMIJE1ll6tD/qgssJ43CX1DiVUTa1eXqkmugL2uQPumGqIrFf3PI kXWP9oNygrWwzlozezxsT6gY0Nxx8hWIvaHplBwHp2wEeB41+t5RKI5Yfc+RSQxgFw7S xhkpjDmpJhTNoXLq/N67CS+Q9m5eKburT1CRDV1w0F3T+oMsxY6fD7C6l0XrJxLM5fw5 mL08gvcKlsR7uT+zSoQnoJc1xTuE29v+nIvGC+54oR+DidU5Gpyd7u7dE6w72RVYWVId HDv4IShi7oh46D/UgXzw/A1XAgblJi1mqlg4VWWnBhbEPm96cD9H6lrJi3M4lGncppYY enWg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=message-id:date:content-transfer-encoding:content-id:mime-version :list-unsubscribe:list-subscribe:list-id:precedence:subject:cc:to :from:organization:dkim-signature; bh=sHnSR+QFwKpo7oWGjVdLyDfof+xSfbIp/ziX7RP5aB4=; fh=t4aTObxHx7IlyGbfV1FqpOLlQUF7nA8IedcEeZI/OuI=; b=POmq/op6DJt/Bn+s+uWsvhTAQVmS8rag3sadtJkJ99NmDjTlZeKVV/OdUSdsopxa+4 Ocn0dffq0uqPm0htAdgaAfp6d9y41q3GrCGmkXLhNsEzw9HLwYAOl5RNpqlnXqc/8Ngk jjbQ79L3D5y6XEB48GI9fpKBvCTa3T2jGb+T6EZs64CcurtgTZCfL65r53ipVi7V91O9 fsEPD9uGc1YxYouh3RPYvvRDyxhRc+7VCZZtBm0PCvwUBihynlq7343ZuaTMgZzFthG6 n1wRObQLOoBoMYjw1FgYQwmSpJPO/mvESvNDzWf974zgnVgWiNabn2pluAk301d47Ses A7Xg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b="RAE3Bp/T"; spf=pass (google.com: domain of linux-kernel+bounces-14555-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-14555-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [147.75.199.223]) by mx.google.com with ESMTPS id s5-20020a05620a0bc500b00781d036a27fsi3070014qki.2.2024.01.02.07.22.11 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Jan 2024 07:22:11 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-14555-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b="RAE3Bp/T"; spf=pass (google.com: domain of linux-kernel+bounces-14555-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-14555-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id C3EFA1C22366 for <ouuuleilei@gmail.com>; Tue, 2 Jan 2024 15:22:11 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 87C3014F77; Tue, 2 Jan 2024 15:21:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="RAE3Bp/T" X-Original-To: linux-kernel@vger.kernel.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 73F5A14A92 for <linux-kernel@vger.kernel.org>; Tue, 2 Jan 2024 15:21:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1704208904; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=sHnSR+QFwKpo7oWGjVdLyDfof+xSfbIp/ziX7RP5aB4=; b=RAE3Bp/TwT/jyrEcWV+C8H7ocSVtRC8f4kFsuUOkNKLHGdB+kd4k7fRqVE7/NPeZCKwefu 7fdXPCtraGP6VEe9s1DncX01mrGUq6uqojxgOtpnIq6tEdIkii9gcSSAvSfA7U9jSQItlo GxuJDsE2dZfO7MpKuyrdABAdrAazL9g= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-141-fpG_uLV2MBefcKkUYRoVag-1; Tue, 02 Jan 2024 10:21:41 -0500 X-MC-Unique: fpG_uLV2MBefcKkUYRoVag-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id CDC013C1E9C6; Tue, 2 Jan 2024 15:21:40 +0000 (UTC) Received: from warthog.procyon.org.uk (unknown [10.42.28.68]) by smtp.corp.redhat.com (Postfix) with ESMTP id D6615492BC6; Tue, 2 Jan 2024 15:21:39 +0000 (UTC) Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 3798903 From: David Howells <dhowells@redhat.com> To: Jeffrey Altman <jaltman@auristor.com> cc: dhowells@redhat.com, Marc Dionne <marc.dionne@auristor.com>, linux-afs@lists.infradead.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] afs: Fix error handling with lookup via FS.InlineBulkStatus Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <140430.1704208899.1@warthog.procyon.org.uk> Content-Transfer-Encoding: quoted-printable Date: Tue, 02 Jan 2024 15:21:39 +0000 Message-ID: <140431.1704208899@warthog.procyon.org.uk> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.9 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1786992585429884693 X-GMAIL-MSGID: 1786992585429884693 |
Series |
afs: Fix error handling with lookup via FS.InlineBulkStatus
|
|
Commit Message
David Howells
Jan. 2, 2024, 3:21 p.m. UTC
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>
cc: 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(-)
Comments
On Tue, Jan 2, 2024 at 11:21 AM David Howells <dhowells@redhat.com> wrote: > > 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> > cc: 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 c14533ef108f..ae563d2a914e 100644 > --- a/fs/afs/dir.c > +++ b/fs/afs/dir.c > @@ -708,6 +708,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; > > @@ -897,12 +899,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 5194b7e6dc8d..ce865ea678d3 100644 > --- a/include/trace/events/afs.h > +++ b/include/trace/events/afs.h > @@ -1102,6 +1102,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), Reviewed-by: Marc Dionne <marc.dionne@auristor.com> Marc
Here's a version of the patch against v6.7-rc7 rather than my afs-fix-rotation
branch.
---
afs: Fix error handling with lookup via FS.InlineBulkStatus
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>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
---
fs/afs/dir.c | 11 ++++++++---
include/trace/events/afs.h | 25 +++++++++++++++++++++++++
2 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 5219182e52e1..a9dcb9d994e4 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -707,6 +707,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;
@@ -895,12 +897,15 @@ 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(op->error);
out_op:
if (op->error == 0) {
- inode = &op->file[1].vnode->netfs.inode;
- op->file[1].vnode = NULL;
+ if (op->file[1].scb.status.abort_code) {
+ op->error = afs_abort_to_error(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 e9d412d19dbb..caec276515dc 100644
--- a/include/trace/events/afs.h
+++ b/include/trace/events/afs.h
@@ -1216,6 +1216,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),
diff --git a/fs/afs/dir.c b/fs/afs/dir.c index c14533ef108f..ae563d2a914e 100644 --- a/fs/afs/dir.c +++ b/fs/afs/dir.c @@ -708,6 +708,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; @@ -897,12 +899,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 5194b7e6dc8d..ce865ea678d3 100644 --- a/include/trace/events/afs.h +++ b/include/trace/events/afs.h @@ -1102,6 +1102,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),