Re: [PATCH v3 1/1] ceph: add getvxattr op

[Xiubo Li <xiubli@redhat.com>]

On 1/17/22 3:36 PM, Milind Changire wrote:
> On Mon, Jan 17, 2022 at 12:49 PM Xiubo Li <xiubli@redhat.com> wrote:
>>
>> On 1/17/22 11:59 AM, Milind Changire wrote:
>>> Problem:
>>> Directory vxattrs like ceph.dir.pin* and ceph.dir.layout* may not be
>>> propagated to the client as frequently to keep them updated. This
>>> creates vxattr availability problems.
>>>
>>> Solution:
>>> Adds new getvxattr op to fetch ceph.dir.pin*, ceph.dir.layout* and
>>> ceph.file.layout* vxattrs.
>>> If the entire layout for a dir or a file is being set, then it is
>>> expected that the layout be set in standard JSON format. Individual
>>> field value retrieval is not wrapped in JSON. The JSON format also
>>> applies while setting the vxattr if the entire layout is being set in
>>> one go.
>>> As a temporary measure, setting a vxattr can also be done in the old
>>> format. The old format will be deprecated in the future.
>>>
>>> URL: https://tracker.ceph.com/issues/51062
>>> Signed-off-by: Milind Changire <mchangir@redhat.com>
>>> ---
>>>    fs/ceph/inode.c              | 51 ++++++++++++++++++++++++++++++++++++
>>>    fs/ceph/mds_client.c         | 27 ++++++++++++++++++-
>>>    fs/ceph/mds_client.h         | 12 ++++++++-
>>>    fs/ceph/strings.c            |  1 +
>>>    fs/ceph/super.h              |  1 +
>>>    fs/ceph/xattr.c              | 34 ++++++++++++++++++++++++
>>>    include/linux/ceph/ceph_fs.h |  1 +
>>>    7 files changed, 125 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>> index e3322fcb2e8d..b63746a7a0e0 100644
>>> --- a/fs/ceph/inode.c
>>> +++ b/fs/ceph/inode.c
>>> @@ -2291,6 +2291,57 @@ int __ceph_do_getattr(struct inode *inode, struct page *locked_page,
>>>        return err;
>>>    }
>>>
>>> +int ceph_do_getvxattr(struct inode *inode, const char *name, void *value,
>>> +                   size_t size)
>>> +{
>>> +     struct ceph_fs_client *fsc = ceph_sb_to_client(inode->i_sb);
>>> +     struct ceph_mds_client *mdsc = fsc->mdsc;
>>> +     struct ceph_mds_request *req;
>>> +     int mode = USE_AUTH_MDS;
>>> +     int err;
>>> +     char *xattr_value;
>>> +     size_t xattr_value_len;
>>> +
>>> +     req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_GETVXATTR, mode);
>>> +     if (IS_ERR(req)) {
>>> +             err = -ENOMEM;
>>> +             goto out;
>>> +     }
>>> +
>>> +     req->r_path2 = kstrdup(name, GFP_NOFS);
>>> +     if (!req->r_path2) {
>>> +             err = -ENOMEM;
>>> +             goto put;
>>> +     }
>>> +
>>> +     ihold(inode);
>>> +     req->r_inode = inode;
>>> +     err = ceph_mdsc_do_request(mdsc, NULL, req);
>>> +     if (err < 0)
>>> +             goto put;
>>> +
>>> +     xattr_value = req->r_reply_info.xattr_info.xattr_value;
>>> +     xattr_value_len = req->r_reply_info.xattr_info.xattr_value_len;
>>> +
>>> +     dout("do_getvxattr xattr_value_len:%u, size:%u\n", xattr_value_len, size);
> Need some help here.
> The kernel CI reported the following warnings:
> 1. for i386 build that size_t is unsigned int
> 2. for riscv build that size_t is unsigned long int
>
> So the above (dout) statement gets a warning either way if I change the expected
> arguments to be either %u or %lu.
>
You can use the %zu, more detail please see 
https://www.kernel.org/doc/Documentation/printk-formats.txt.



>>> +
>>> +     err = xattr_value_len;
>>> +     if (size == 0)
>>> +             goto put;
>>> +
>>> +     if (xattr_value_len > size) {
>>> +             err = -ERANGE;
>>> +             goto put;
>>> +     }
>>> +
[...]
>>> +                     return false;
>>> +     }
>>> +     return true;
>>> +}
>>> +
>>>    ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,
>>>                      size_t size)
>>>    {
>>> @@ -927,6 +951,16 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,
>>>        int req_mask;
>>>        ssize_t err;
>>>
>>> +     if (!strncmp(name, XATTR_CEPH_PREFIX, XATTR_CEPH_PREFIX_LEN) &&
>>> +         ceph_cluster_has_feature(inode, CEPHFS_FEATURE_GETVXATTR)) {
>>> +             err = ceph_do_getvxattr(inode, name, value, size);
>>> +             /* if cluster doesn't support xattr, we try to service it
>>> +              * locally
>>> +              */
>>> +             if (err >= 0)
>>> +                     return err;
>>> +     }
>>> +
>> If the 'Fa' or 'Fr' caps is issued to kclient, could we get this vxattr
>> locally instead of getting it from MDS every time ?
> The new mechanism is meant to supersede the old one.
> 1. The new layout output format is JSON
> 2. The new mechanism also recursively resolves the layout to the
> closest ancestor
> 3. The new mechanism deals with ceph vxattrs exclusively at the server end
>     (currently getvxattr handles only a limited subset of all the vxattrs)
> 4. There's no way to fetch ceph.dir.pin* vxattrs locally
> (see https://github.com/ceph/ceph/pull/42001 for userspace work)
>
Okay.


>>
>>
>>>        /* let's see if a virtual xattr was requested */
>>>        vxattr = ceph_match_vxattr(inode, name);
>>>        if (vxattr) {
>>> diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h
>>> index 7ad6c3d0db7d..66db21ac5f0c 100644
>>> --- a/include/linux/ceph/ceph_fs.h
>>> +++ b/include/linux/ceph/ceph_fs.h
>>> @@ -328,6 +328,7 @@ enum {
>>>        CEPH_MDS_OP_LOOKUPPARENT = 0x00103,
>>>        CEPH_MDS_OP_LOOKUPINO  = 0x00104,
>>>        CEPH_MDS_OP_LOOKUPNAME = 0x00105,
>>> +     CEPH_MDS_OP_GETVXATTR  = 0x00106,
>>>
>>>        CEPH_MDS_OP_SETXATTR   = 0x01105,
>>>        CEPH_MDS_OP_RMXATTR    = 0x01106,