* [v3] ceph: if we are blacklisted, __do_request returns directly
@ 2020-04-17 11:07 Yanhu Cao
2020-04-20 12:16 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Yanhu Cao @ 2020-04-17 11:07 UTC (permalink / raw)
To: jlayton; +Cc: sage, idryomov, ceph-devel, linux-kernel, Yanhu Cao
If we mount cephfs by the recover_session option,
__do_request can return directly until the client automatically reconnects.
Signed-off-by: Yanhu Cao <gmayyyha@gmail.com>
---
fs/ceph/mds_client.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 486f91f9685b..16ac5e5f7f79 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2708,6 +2708,12 @@ static void __do_request(struct ceph_mds_client *mdsc,
put_request_session(req);
+ if (mdsc->fsc->blacklisted &&
+ ceph_test_mount_opt(mdsc->fsc, CLEANRECOVER)) {
+ err = -EBLACKLISTED;
+ goto finish;
+ }
+
mds = __choose_mds(mdsc, req, &random);
if (mds < 0 ||
ceph_mdsmap_get_state(mdsc->mdsmap, mds) < CEPH_MDS_STATE_ACTIVE) {
--
2.24.2 (Apple Git-127)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [v3] ceph: if we are blacklisted, __do_request returns directly
2020-04-17 11:07 [v3] ceph: if we are blacklisted, __do_request returns directly Yanhu Cao
@ 2020-04-20 12:16 ` Jeff Layton
2020-04-21 2:13 ` Yanhu Cao
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2020-04-20 12:16 UTC (permalink / raw)
To: Yanhu Cao; +Cc: sage, idryomov, ceph-devel, linux-kernel
On Fri, 2020-04-17 at 19:07 +0800, Yanhu Cao wrote:
> If we mount cephfs by the recover_session option,
> __do_request can return directly until the client automatically reconnects.
>
> Signed-off-by: Yanhu Cao <gmayyyha@gmail.com>
> ---
> fs/ceph/mds_client.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 486f91f9685b..16ac5e5f7f79 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2708,6 +2708,12 @@ static void __do_request(struct ceph_mds_client *mdsc,
>
> put_request_session(req);
>
> + if (mdsc->fsc->blacklisted &&
> + ceph_test_mount_opt(mdsc->fsc, CLEANRECOVER)) {
> + err = -EBLACKLISTED;
> + goto finish;
> + }
> +
Why check for CLEANRECOVER? If we're mounted with recover_session=no
wouldn't we want to do the same thing here?
Either way, it's still blacklisted. The only difference is that it won't
attempt to automatically recover the session that way.
> mds = __choose_mds(mdsc, req, &random);
> if (mds < 0 ||
> ceph_mdsmap_get_state(mdsc->mdsmap, mds) < CEPH_MDS_STATE_ACTIVE) {
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v3] ceph: if we are blacklisted, __do_request returns directly
2020-04-20 12:16 ` Jeff Layton
@ 2020-04-21 2:13 ` Yanhu Cao
2020-04-21 10:15 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Yanhu Cao @ 2020-04-21 2:13 UTC (permalink / raw)
To: Jeff Layton; +Cc: Sage Weil, Ilya Dryomov, ceph-devel, LKML
On Mon, Apr 20, 2020 at 8:16 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Fri, 2020-04-17 at 19:07 +0800, Yanhu Cao wrote:
> > If we mount cephfs by the recover_session option,
> > __do_request can return directly until the client automatically reconnects.
> >
> > Signed-off-by: Yanhu Cao <gmayyyha@gmail.com>
> > ---
> > fs/ceph/mds_client.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 486f91f9685b..16ac5e5f7f79 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -2708,6 +2708,12 @@ static void __do_request(struct ceph_mds_client *mdsc,
> >
> > put_request_session(req);
> >
> > + if (mdsc->fsc->blacklisted &&
> > + ceph_test_mount_opt(mdsc->fsc, CLEANRECOVER)) {
> > + err = -EBLACKLISTED;
> > + goto finish;
> > + }
> > +
>
> Why check for CLEANRECOVER? If we're mounted with recover_session=no
> wouldn't we want to do the same thing here?
>
> Either way, it's still blacklisted. The only difference is that it won't
> attempt to automatically recover the session that way.
I think mds will clear the blacklist. In addition to loading cephfs
via recover_session=clean, I didn't find a location where
fsc->blacklisted is set to false. If the client has been blacklisted,
should it always be blacklisted (fsc->blacklisted=true)? Or is there
another way to set fsc->blacklised to false?
>
>
> > mds = __choose_mds(mdsc, req, &random);
> > if (mds < 0 ||
> > ceph_mdsmap_get_state(mdsc->mdsmap, mds) < CEPH_MDS_STATE_ACTIVE) {
> --
> Jeff Layton <jlayton@kernel.org>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v3] ceph: if we are blacklisted, __do_request returns directly
2020-04-21 2:13 ` Yanhu Cao
@ 2020-04-21 10:15 ` Jeff Layton
2020-04-21 12:21 ` Yanhu Cao
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2020-04-21 10:15 UTC (permalink / raw)
To: Yanhu Cao; +Cc: Sage Weil, Ilya Dryomov, ceph-devel, LKML
On Tue, 2020-04-21 at 10:13 +0800, Yanhu Cao wrote:
> On Mon, Apr 20, 2020 at 8:16 PM Jeff Layton <jlayton@kernel.org> wrote:
> > On Fri, 2020-04-17 at 19:07 +0800, Yanhu Cao wrote:
> > > If we mount cephfs by the recover_session option,
> > > __do_request can return directly until the client automatically reconnects.
> > >
> > > Signed-off-by: Yanhu Cao <gmayyyha@gmail.com>
> > > ---
> > > fs/ceph/mds_client.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > index 486f91f9685b..16ac5e5f7f79 100644
> > > --- a/fs/ceph/mds_client.c
> > > +++ b/fs/ceph/mds_client.c
> > > @@ -2708,6 +2708,12 @@ static void __do_request(struct ceph_mds_client *mdsc,
> > >
> > > put_request_session(req);
> > >
> > > + if (mdsc->fsc->blacklisted &&
> > > + ceph_test_mount_opt(mdsc->fsc, CLEANRECOVER)) {
> > > + err = -EBLACKLISTED;
> > > + goto finish;
> > > + }
> > > +
> >
> > Why check for CLEANRECOVER? If we're mounted with recover_session=no
> > wouldn't we want to do the same thing here?
> >
> > Either way, it's still blacklisted. The only difference is that it won't
> > attempt to automatically recover the session that way.
>
> I think mds will clear the blacklist. In addition to loading cephfs
> via recover_session=clean, I didn't find a location where
> fsc->blacklisted is set to false. If the client has been blacklisted,
> should it always be blacklisted (fsc->blacklisted=true)? Or is there
> another way to set fsc->blacklised to false?
>
Basically, this patch is just changing it so that when the client is
blacklisted and the mount is done with recover_session=clean, we'll
shortcut the rest of the __do_request and just return -EBLACKLISTED.
My question is: why do we need to test for recover_session=clean here?
If the client _knows_ that it is blacklisted, why would it want to
continue with __do_request in the recover_session=no case? Would it make
more sense to always return early in __do_request when the client is
blacklisted?
> >
> > > mds = __choose_mds(mdsc, req, &random);
> > > if (mds < 0 ||
> > > ceph_mdsmap_get_state(mdsc->mdsmap, mds) < CEPH_MDS_STATE_ACTIVE) {
> > --
> > Jeff Layton <jlayton@kernel.org>
> >
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v3] ceph: if we are blacklisted, __do_request returns directly
2020-04-21 10:15 ` Jeff Layton
@ 2020-04-21 12:21 ` Yanhu Cao
2020-04-23 11:04 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Yanhu Cao @ 2020-04-21 12:21 UTC (permalink / raw)
To: Jeff Layton; +Cc: Sage Weil, Ilya Dryomov, ceph-devel, LKML
On Tue, Apr 21, 2020 at 6:15 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2020-04-21 at 10:13 +0800, Yanhu Cao wrote:
> > On Mon, Apr 20, 2020 at 8:16 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > On Fri, 2020-04-17 at 19:07 +0800, Yanhu Cao wrote:
> > > > If we mount cephfs by the recover_session option,
> > > > __do_request can return directly until the client automatically reconnects.
> > > >
> > > > Signed-off-by: Yanhu Cao <gmayyyha@gmail.com>
> > > > ---
> > > > fs/ceph/mds_client.c | 6 ++++++
> > > > 1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > > index 486f91f9685b..16ac5e5f7f79 100644
> > > > --- a/fs/ceph/mds_client.c
> > > > +++ b/fs/ceph/mds_client.c
> > > > @@ -2708,6 +2708,12 @@ static void __do_request(struct ceph_mds_client *mdsc,
> > > >
> > > > put_request_session(req);
> > > >
> > > > + if (mdsc->fsc->blacklisted &&
> > > > + ceph_test_mount_opt(mdsc->fsc, CLEANRECOVER)) {
> > > > + err = -EBLACKLISTED;
> > > > + goto finish;
> > > > + }
> > > > +
> > >
> > > Why check for CLEANRECOVER? If we're mounted with recover_session=no
> > > wouldn't we want to do the same thing here?
> > >
> > > Either way, it's still blacklisted. The only difference is that it won't
> > > attempt to automatically recover the session that way.
> >
> > I think mds will clear the blacklist. In addition to loading cephfs
> > via recover_session=clean, I didn't find a location where
> > fsc->blacklisted is set to false. If the client has been blacklisted,
> > should it always be blacklisted (fsc->blacklisted=true)? Or is there
> > another way to set fsc->blacklised to false?
> >
>
> Basically, this patch is just changing it so that when the client is
> blacklisted and the mount is done with recover_session=clean, we'll
> shortcut the rest of the __do_request and just return -EBLACKLISTED.
>
> My question is: why do we need to test for recover_session=clean here?
I thought that fsc->blacklisted is related to recovery_session=clean.
If we test it, the client can do the rest of __do_request. It seems
useless now because kcephfs cannot resume the session like ceph-fuse
when mds cleared the blacklist.
>
> If the client _knows_ that it is blacklisted, why would it want to
> continue with __do_request in the recover_session=no case? Would it make
> more sense to always return early in __do_request when the client is
> blacklisted?
Makes sense. if there is no problem. I will patch the next commit and
return -EBLACKLISTED only when fsc->blacklisted=true.
>
>
> > >
> > > > mds = __choose_mds(mdsc, req, &random);
> > > > if (mds < 0 ||
> > > > ceph_mdsmap_get_state(mdsc->mdsmap, mds) < CEPH_MDS_STATE_ACTIVE) {
> > > --
> > > Jeff Layton <jlayton@kernel.org>
> > >
>
> --
> Jeff Layton <jlayton@kernel.org>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v3] ceph: if we are blacklisted, __do_request returns directly
2020-04-21 12:21 ` Yanhu Cao
@ 2020-04-23 11:04 ` Jeff Layton
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Layton @ 2020-04-23 11:04 UTC (permalink / raw)
To: Yanhu Cao; +Cc: Sage Weil, Ilya Dryomov, ceph-devel, LKML
On Tue, 2020-04-21 at 20:21 +0800, Yanhu Cao wrote:
> On Tue, Apr 21, 2020 at 6:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > On Tue, 2020-04-21 at 10:13 +0800, Yanhu Cao wrote:
> > > On Mon, Apr 20, 2020 at 8:16 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > On Fri, 2020-04-17 at 19:07 +0800, Yanhu Cao wrote:
> > > > > If we mount cephfs by the recover_session option,
> > > > > __do_request can return directly until the client automatically reconnects.
> > > > >
> > > > > Signed-off-by: Yanhu Cao <gmayyyha@gmail.com>
> > > > > ---
> > > > > fs/ceph/mds_client.c | 6 ++++++
> > > > > 1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > > > index 486f91f9685b..16ac5e5f7f79 100644
> > > > > --- a/fs/ceph/mds_client.c
> > > > > +++ b/fs/ceph/mds_client.c
> > > > > @@ -2708,6 +2708,12 @@ static void __do_request(struct ceph_mds_client *mdsc,
> > > > >
> > > > > put_request_session(req);
> > > > >
> > > > > + if (mdsc->fsc->blacklisted &&
> > > > > + ceph_test_mount_opt(mdsc->fsc, CLEANRECOVER)) {
> > > > > + err = -EBLACKLISTED;
> > > > > + goto finish;
> > > > > + }
> > > > > +
> > > >
> > > > Why check for CLEANRECOVER? If we're mounted with recover_session=no
> > > > wouldn't we want to do the same thing here?
> > > >
> > > > Either way, it's still blacklisted. The only difference is that it won't
> > > > attempt to automatically recover the session that way.
> > >
> > > I think mds will clear the blacklist. In addition to loading cephfs
> > > via recover_session=clean, I didn't find a location where
> > > fsc->blacklisted is set to false. If the client has been blacklisted,
> > > should it always be blacklisted (fsc->blacklisted=true)? Or is there
> > > another way to set fsc->blacklised to false?
> > >
> >
> > Basically, this patch is just changing it so that when the client is
> > blacklisted and the mount is done with recover_session=clean, we'll
> > shortcut the rest of the __do_request and just return -EBLACKLISTED.
> >
> > My question is: why do we need to test for recover_session=clean here?
>
> I thought that fsc->blacklisted is related to recovery_session=clean.
> If we test it, the client can do the rest of __do_request. It seems
> useless now because kcephfs cannot resume the session like ceph-fuse
> when mds cleared the blacklist.
>
fsc->blacklisted just indicates that the client has detected that it has
been blacklisted. With recover_session=clean it can reconnect and
continue on (with some limitations). See:
https://ceph.io/community/automatic-cephfs-recovery-after-blacklisting/
> > If the client _knows_ that it is blacklisted, why would it want to
> > continue with __do_request in the recover_session=no case? Would it make
> > more sense to always return early in __do_request when the client is
> > blacklisted?
>
> Makes sense. if there is no problem. I will patch the next commit and
> return -EBLACKLISTED only when fsc->blacklisted=true.
>
Sure. To be clear, I don't see this as a bug, but rather just an
optimization. If the client is blacklisted then we don't really need to
do all of the work or attempt to send the request until that's been
cleared. That's the case regardless of the recover_session= option.
> >
> > > > > mds = __choose_mds(mdsc, req, &random);
> > > > > if (mds < 0 ||
> > > > > ceph_mdsmap_get_state(mdsc->mdsmap, mds) < CEPH_MDS_STATE_ACTIVE) {
> > > > --
> > > > Jeff Layton <jlayton@kernel.org>
> > > >
> >
> > --
> > Jeff Layton <jlayton@kernel.org>
> >
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-23 11:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-17 11:07 [v3] ceph: if we are blacklisted, __do_request returns directly Yanhu Cao
2020-04-20 12:16 ` Jeff Layton
2020-04-21 2:13 ` Yanhu Cao
2020-04-21 10:15 ` Jeff Layton
2020-04-21 12:21 ` Yanhu Cao
2020-04-23 11:04 ` Jeff Layton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).