| Message ID | 877ep46ei2.fsf@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On Apr 18, 2018, at 17:38, Luis Henriques <lhenriques@suse.com> wrote: > > "Yan, Zheng" <zyan@redhat.com> writes: > >>> On Apr 17, 2018, at 21:46, Luis Henriques <lhenriques@suse.com> wrote: >>> >>> Hi! >>> >>> I see the following commit is queued for the kernel cephfs client: >>> >>> commit e2311baa73a883eb8501c895cc817d3c96c3b896 >>> Author: Yan, Zheng <zyan@redhat.com> >>> Date: Sun Apr 8 09:54:39 2018 +0800 >>> >>> ceph: check if mds create snaprealm when setting quota >>> >>> If mds does not, return -EOPNOTSUPP. >>> >>> [ This is related with http://tracker.ceph.com/issues/23491 ] >>> >>> However, I'm not sure it is doing the right thing (although I confess >>> I'm not sure either what the right thing would be...) >>> >>> This commit is doing 2 things: >>> >>> 1) it returns an error if a user tries to set quotas and the MDS doesn't >>> support it (using the new snaprealm method) >>> >>> 2) it forbids reading the ceph.quota.* xattrs, even if they're set >>> >>> Regarding 1), it's a change in behaviour from previous kernels -- older >>> kernels allow these attributes to be set (but not read!), even if they >>> don't really support quotas. Also, the error that is returned >>> (-EOPNOTSUPP) is a bit misleading as the xattr is actually set in the >>> directory. >> >> any better error code? > > The error code is fine, my complain was about returning an error for an > operation that actually succeeded: if a different (fuse) client does a > getfattr it will see that the attribute was set and it *will* have > quotas enabled (at least the old versions of the fuse client, I haven't > check if this has changed in new versions). > >>> >>> Regarding 2), it's a bit artificial to forbid the user to get these >>> attributes as we do actually have them available. >> >> If attributes have no effect, why can’t we treat them as nonexistence. > > True, the attribute does not have an effect for *this* client. > >> >>> >>> I understand the rational behind this patch, but since quotas won't be >>> enforced anyway in this scenario, is there really any value added with >>> this patch? >>> >>> I would suggest 2 alternatives: >>> >>> 1. check cluster version and return the error in setxattr if version < >>> Mimic (but maybe still allow reading the xattrs?) >>> >> >> The problem is we run out of feature bits. No straight way to do the check >> > > Ah! I missed that :-/ > >> >>> 2. do nothing -- if we're using a cluster that does not support quotas >>> anyway, maybe there's no point in having these different behaviours >>> for old and new clients. >>> >> >> This is confusing. we reclaim that > 4.16 kernel supports quota. I don’t think it’s a good that >> user successfully set quota on > 4.16 kernel, but find that quota has no effect. > > Again, this is not completely true -- old fuse clients will see the side > effects of this operation. > >> 4.16 kernel is the first one that support quota, does behavior change really matter? > > Right, I don't really have any strong feelings against this patch. As I > said, my preference would be one of the 2 alternatives above, but I > guess that at least we'll need to update doc/cephfs/quota.rst (see > suggestion bellow). > > Cheers, > -- > Luis > > From 5a034c3d43e4828e2caaa6156f067155ca2f6c39 Mon Sep 17 00:00:00 2001 > From: Luis Henriques <lhenriques@suse.com> > Date: Wed, 18 Apr 2018 10:28:55 +0100 > Subject: [PATCH] doc/cephfs: update kernel client quotas support info > > Signed-off-by: Luis Henriques <lhenriques@suse.com> > --- > doc/cephfs/quota.rst | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/doc/cephfs/quota.rst b/doc/cephfs/quota.rst > index aad0e0bbf0c2..60e826b40139 100644 > --- a/doc/cephfs/quota.rst > +++ b/doc/cephfs/quota.rst > @@ -23,9 +23,12 @@ Limitations > of data. Generally speaking writers will be stopped within 10s of > seconds of crossing the configured limit. > > -#. *Quotas are not yet implemented in the kernel client.* Quotas are > - supported by the userspace client (libcephfs, ceph-fuse) but are > - not yet implemented in the Linux kernel client. > +#. *Quotas are implemented in the kernel client > 4.16 only.* Quotas > + are supported by the userspace client (libcephfs, ceph-fuse). > + Linux kernel clients > 4.16 support CephFS quotas but only on > + mimic+ clusters. Kernel clients (even recent versions) will fail > + to handle quotas on older clusters, even if they may be able to set > + the quotas extended attributes. > > #. *Quotas must be configured carefully when used with path-based > mount restrictions.* The client needs to have access to the Could you open a RP for this change Thanks Yan, Zheng -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"Yan, Zheng" <zyan@redhat.com> writes: <snip> > > Could you open a RP for this change > Done, that's PR#21499 Cheers,
diff --git a/doc/cephfs/quota.rst b/doc/cephfs/quota.rst index aad0e0bbf0c2..60e826b40139 100644 --- a/doc/cephfs/quota.rst +++ b/doc/cephfs/quota.rst @@ -23,9 +23,12 @@ Limitations of data. Generally speaking writers will be stopped within 10s of seconds of crossing the configured limit. -#. *Quotas are not yet implemented in the kernel client.* Quotas are - supported by the userspace client (libcephfs, ceph-fuse) but are - not yet implemented in the Linux kernel client. +#. *Quotas are implemented in the kernel client > 4.16 only.* Quotas + are supported by the userspace client (libcephfs, ceph-fuse). + Linux kernel clients > 4.16 support CephFS quotas but only on + mimic+ clusters. Kernel clients (even recent versions) will fail + to handle quotas on older clusters, even if they may be able to set + the quotas extended attributes. #. *Quotas must be configured carefully when used with path-based mount restrictions.* The client needs to have access to the