Re: [RFC PATCH v4 17/17] ceph: add fscrypt ioctls

Luis Henriques
2021-01-28T14:09:56Z

Jeff Layton <jlayton@kernel.org> writes:

> On Thu, 2021-01-28 at 12:22 +0000, Luis Henriques wrote:
>> Jeff Layton <jlayton@kernel.org> writes:
>>
>> > Most of the ioctls, we gate on the MDS feature support. The exception is
>> > the key removal and status functions that we still want to work if the
>> > MDS's were to (inexplicably) lose the feature.
>> >
>> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> > ---
>> >  fs/ceph/ioctl.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 61 insertions(+)
>> >
>> > diff --git a/fs/ceph/ioctl.c b/fs/ceph/ioctl.c
>> > index 6e061bf62ad4..832909f3eb1b 100644
>> > --- a/fs/ceph/ioctl.c
>> > +++ b/fs/ceph/ioctl.c
>> > @@ -6,6 +6,7 @@
>> >  #include "mds_client.h"
>> >  #include "ioctl.h"
>> >  #include <linux/ceph/striper.h>
>> > +#include <linux/fscrypt.h>
>> >
>> >
>> >
>> >
>> >  /*
>> >   * ioctls
>> > @@ -268,8 +269,29 @@ static long ceph_ioctl_syncio(struct file *file)
>> >   return 0;
>> >  }
>> >
>> >
>> >
>> >
>> > +static int vet_mds_for_fscrypt(struct file *file)
>> > +{
>> > + int i, ret = -EOPNOTSUPP;
>> > + struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(file_inode(file)->i_sb);
>> > +
>> > + mutex_lock(&mdsc->mutex);
>> > + for (i = 0; i < mdsc->max_sessions; i++) {
>> > + struct ceph_mds_session *s = __ceph_lookup_mds_session(mdsc, i);
>> > +
>> > + if (!s)
>> > + continue;
>> > + if (test_bit(CEPHFS_FEATURE_ALTERNATE_NAME, &s->s_features))
>> > + ret = 0;
>>
>> And another one, I believe...? We need this here:
>>
>> ceph_put_mds_session(s);
>>
>
> Well spotted. Though since we hold the mutex over the whole thing, I
> probably should change this to just not take references at all. I'll fix
> that.
>
>> Also, isn't this logic broken? Shouldn't we walk through all the sessions
>> and return 0 only if they all have that feature bit set?
>>
>
> Tough call.
>
> AFAIU, we're not guaranteed to have a session with all of the available
> MDS's at any given time. I figured we'd have one and we'd assume that
> all of the others would be similar.
>
> I'm not sure if that's a safe assumption or not though. Let me do some
> asking around...

Yeah, you're probably right. All the sessions should have the same
features set.

Cheers,
--
Luis

> Thanks!
> --
> Jeff Layton <jlayton@kernel.org>
>