[RFC PATCH] ceph: try to prevent exceeding the MDS maximum xattr size

[RFC PATCH] ceph: try to prevent exceeding the MDS maximum xattr size

[Luís Henriques]

The MDS tries to enforce a limit on the total key/values in extended
attributes.  However, this limit is enforced only if doing a synchronous
operation (MDS_OP_SETXATTR) -- if we're buffering the xattrs, the MDS
doesn't have a chance to enforce these limits.

This patch forces the usage of the synchronous operation if xattrs size hits
the maximum size that is set on the MDS by default (64k).

While there, fix a dout() that would trigger a printk warning:

[   98.718078] ------------[ cut here ]------------
[   98.719012] precision 65536 too large
[   98.719039] WARNING: CPU: 1 PID: 3755 at lib/vsprintf.c:2703 vsnprintf+0x5e3/0x600
...

URL: https://tracker.ceph.com/issues/55725
Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 fs/ceph/xattr.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index afec84088471..09751a5f028c 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -15,6 +15,12 @@
 #define XATTR_CEPH_PREFIX "ceph."
 #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
 
+/*
+ * Maximum size of xattrs the MDS can handle per inode by default.  This
+ * includes the attribute name and 4+4 bytes for the key/value sizes.
+ */
+#define MDS_MAX_XATTR_PAIRS_SIZE (1<<16) /* 64K */
+
 static int __remove_xattr(struct ceph_inode_info *ci,
 			  struct ceph_inode_xattr *xattr);
 
@@ -1078,7 +1084,7 @@ static int ceph_sync_setxattr(struct inode *inode, const char *name,
 			flags |= CEPH_XATTR_REMOVE;
 	}
 
-	dout("setxattr value=%.*s\n", (int)size, value);
+	dout("setxattr value size: ld\n", size);
 
 	/* do request */
 	req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
@@ -1176,8 +1182,13 @@ int __ceph_setxattr(struct inode *inode, const char *name,
 	spin_lock(&ci->i_ceph_lock);
 retry:
 	issued = __ceph_caps_issued(ci, NULL);
-	if (ci->i_xattrs.version == 0 || !(issued & CEPH_CAP_XATTR_EXCL))
+	required_blob_size = __get_required_blob_size(ci, name_len, val_len);
+	if ((ci->i_xattrs.version == 0) || !(issued & CEPH_CAP_XATTR_EXCL) ||
+	    (required_blob_size >= MDS_MAX_XATTR_PAIRS_SIZE)) {
+		dout("%s do sync setxattr: version: %llu blob size: %d\n",
+		     __func__, ci->i_xattrs.version, required_blob_size);
 		goto do_sync;
+	}
 
 	if (!lock_snap_rwsem && !ci->i_head_snapc) {
 		lock_snap_rwsem = true;
@@ -1193,8 +1204,6 @@ int __ceph_setxattr(struct inode *inode, const char *name,
 	     ceph_cap_string(issued));
 	__build_xattrs(inode);
 
-	required_blob_size = __get_required_blob_size(ci, name_len, val_len);
-
 	if (!ci->i_xattrs.prealloc_blob ||
 	    required_blob_size > ci->i_xattrs.prealloc_blob->alloc_len) {
 		struct ceph_buffer *blob;

Re: [RFC PATCH] ceph: try to prevent exceeding the MDS maximum xattr size

[Luís Henriques]

Luís Henriques <lhenriques@suse.de> writes:

> The MDS tries to enforce a limit on the total key/values in extended
> attributes.  However, this limit is enforced only if doing a synchronous
> operation (MDS_OP_SETXATTR) -- if we're buffering the xattrs, the MDS
> doesn't have a chance to enforce these limits.
>
> This patch forces the usage of the synchronous operation if xattrs size hits
> the maximum size that is set on the MDS by default (64k).
>
> While there, fix a dout() that would trigger a printk warning:
>
> [   98.718078] ------------[ cut here ]------------
> [   98.719012] precision 65536 too large
> [   98.719039] WARNING: CPU: 1 PID: 3755 at lib/vsprintf.c:2703 vsnprintf+0x5e3/0x600
> ...
>
> URL: https://tracker.ceph.com/issues/55725
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>  fs/ceph/xattr.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index afec84088471..09751a5f028c 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -15,6 +15,12 @@
>  #define XATTR_CEPH_PREFIX "ceph."
>  #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
>  
> +/*
> + * Maximum size of xattrs the MDS can handle per inode by default.  This
> + * includes the attribute name and 4+4 bytes for the key/value sizes.
> + */
> +#define MDS_MAX_XATTR_PAIRS_SIZE (1<<16) /* 64K */
> +
>  static int __remove_xattr(struct ceph_inode_info *ci,
>  			  struct ceph_inode_xattr *xattr);
>  
> @@ -1078,7 +1084,7 @@ static int ceph_sync_setxattr(struct inode *inode, const char *name,
>  			flags |= CEPH_XATTR_REMOVE;
>  	}
>  
> -	dout("setxattr value=%.*s\n", (int)size, value);
> +	dout("setxattr value size: ld\n", size);

Oops!  Looks like someone ate a '%' char.  Oh well, I won't bother sending
out a new version for now as this is an RFC and the MDS side is what
really needs fixing.  In fact, the client-side may be something very
different from this RFC.

Cheers,
-- 
Luís

>  
>  	/* do request */
>  	req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
> @@ -1176,8 +1182,13 @@ int __ceph_setxattr(struct inode *inode, const char *name,
>  	spin_lock(&ci->i_ceph_lock);
>  retry:
>  	issued = __ceph_caps_issued(ci, NULL);
> -	if (ci->i_xattrs.version == 0 || !(issued & CEPH_CAP_XATTR_EXCL))
> +	required_blob_size = __get_required_blob_size(ci, name_len, val_len);
> +	if ((ci->i_xattrs.version == 0) || !(issued & CEPH_CAP_XATTR_EXCL) ||
> +	    (required_blob_size >= MDS_MAX_XATTR_PAIRS_SIZE)) {
> +		dout("%s do sync setxattr: version: %llu blob size: %d\n",
> +		     __func__, ci->i_xattrs.version, required_blob_size);
>  		goto do_sync;
> +	}
>  
>  	if (!lock_snap_rwsem && !ci->i_head_snapc) {
>  		lock_snap_rwsem = true;
> @@ -1193,8 +1204,6 @@ int __ceph_setxattr(struct inode *inode, const char *name,
>  	     ceph_cap_string(issued));
>  	__build_xattrs(inode);
>  
> -	required_blob_size = __get_required_blob_size(ci, name_len, val_len);
> -
>  	if (!ci->i_xattrs.prealloc_blob ||
>  	    required_blob_size > ci->i_xattrs.prealloc_blob->alloc_len) {
>  		struct ceph_buffer *blob;

Re: [RFC PATCH] ceph: try to prevent exceeding the MDS maximum xattr size

[kernel test robot]

Hi "Luís,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on ceph-client/for-linus]
[also build test WARNING on v5.18-rc7 next-20220520]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Lu-s-Henriques/ceph-try-to-prevent-exceeding-the-MDS-maximum-xattr-size/20220520-195559
base:   https://github.com/ceph/ceph-client.git for-linus
config: i386-randconfig-a006 (https://download.01.org/0day-ci/archive/20220521/202205210021.IgsLVawx-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project e00cbbec06c08dc616a0d52a20f678b8fbd4e304)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/3c9e88c091f0e9b17856d7de9b8c705e827ee716
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lu-s-Henriques/ceph-try-to-prevent-exceeding-the-MDS-maximum-xattr-size/20220520-195559
        git checkout 3c9e88c091f0e9b17856d7de9b8c705e827ee716
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/ceph/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/ceph/xattr.c:1087:36: warning: data argument not used by format string [-Wformat-extra-args]
           dout("setxattr value size: ld\n", size);
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
   include/linux/ceph/ceph_debug.h:35:45: note: expanded from macro 'dout'
   # define dout(fmt, ...) pr_debug(" " fmt, ##__VA_ARGS__)
                           ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
   include/linux/printk.h:576:38: note: expanded from macro 'pr_debug'
           no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
   include/linux/printk.h:132:17: note: expanded from macro 'no_printk'
                   printk(fmt, ##__VA_ARGS__);             \
                   ~~~~~~~~~~~~~~^~~~~~~~~~~~
   include/linux/printk.h:446:60: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
   include/linux/printk.h:418:19: note: expanded from macro 'printk_index_wrap'
                   _p_func(_fmt, ##__VA_ARGS__);                           \
                           ~~~~    ^
   1 warning generated.


vim +1087 fs/ceph/xattr.c

  1058	
  1059	static int ceph_sync_setxattr(struct inode *inode, const char *name,
  1060				      const char *value, size_t size, int flags)
  1061	{
  1062		struct ceph_fs_client *fsc = ceph_sb_to_client(inode->i_sb);
  1063		struct ceph_inode_info *ci = ceph_inode(inode);
  1064		struct ceph_mds_request *req;
  1065		struct ceph_mds_client *mdsc = fsc->mdsc;
  1066		struct ceph_osd_client *osdc = &fsc->client->osdc;
  1067		struct ceph_pagelist *pagelist = NULL;
  1068		int op = CEPH_MDS_OP_SETXATTR;
  1069		int err;
  1070	
  1071		if (size > 0) {
  1072			/* copy value into pagelist */
  1073			pagelist = ceph_pagelist_alloc(GFP_NOFS);
  1074			if (!pagelist)
  1075				return -ENOMEM;
  1076	
  1077			err = ceph_pagelist_append(pagelist, value, size);
  1078			if (err)
  1079				goto out;
  1080		} else if (!value) {
  1081			if (flags & CEPH_XATTR_REPLACE)
  1082				op = CEPH_MDS_OP_RMXATTR;
  1083			else
  1084				flags |= CEPH_XATTR_REMOVE;
  1085		}
  1086	
> 1087		dout("setxattr value size: ld\n", size);
  1088	
  1089		/* do request */
  1090		req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
  1091		if (IS_ERR(req)) {
  1092			err = PTR_ERR(req);
  1093			goto out;
  1094		}
  1095	
  1096		req->r_path2 = kstrdup(name, GFP_NOFS);
  1097		if (!req->r_path2) {
  1098			ceph_mdsc_put_request(req);
  1099			err = -ENOMEM;
  1100			goto out;
  1101		}
  1102	
  1103		if (op == CEPH_MDS_OP_SETXATTR) {
  1104			req->r_args.setxattr.flags = cpu_to_le32(flags);
  1105			req->r_args.setxattr.osdmap_epoch =
  1106				cpu_to_le32(osdc->osdmap->epoch);
  1107			req->r_pagelist = pagelist;
  1108			pagelist = NULL;
  1109		}
  1110	
  1111		req->r_inode = inode;
  1112		ihold(inode);
  1113		req->r_num_caps = 1;
  1114		req->r_inode_drop = CEPH_CAP_XATTR_SHARED;
  1115	
  1116		dout("xattr.ver (before): %lld\n", ci->i_xattrs.version);
  1117		err = ceph_mdsc_do_request(mdsc, NULL, req);
  1118		ceph_mdsc_put_request(req);
  1119		dout("xattr.ver (after): %lld\n", ci->i_xattrs.version);
  1120	
  1121	out:
  1122		if (pagelist)
  1123			ceph_pagelist_release(pagelist);
  1124		return err;
  1125	}
  1126	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [RFC PATCH] ceph: try to prevent exceeding the MDS maximum xattr size

[Xiubo Li]

On 5/20/22 7:54 PM, Luís Henriques wrote:
> The MDS tries to enforce a limit on the total key/values in extended
> attributes.  However, this limit is enforced only if doing a synchronous
> operation (MDS_OP_SETXATTR) -- if we're buffering the xattrs, the MDS
> doesn't have a chance to enforce these limits.
>
> This patch forces the usage of the synchronous operation if xattrs size hits
> the maximum size that is set on the MDS by default (64k).
>
> While there, fix a dout() that would trigger a printk warning:
>
> [   98.718078] ------------[ cut here ]------------
> [   98.719012] precision 65536 too large
> [   98.719039] WARNING: CPU: 1 PID: 3755 at lib/vsprintf.c:2703 vsnprintf+0x5e3/0x600
> ...
>
> URL: https://tracker.ceph.com/issues/55725
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>   fs/ceph/xattr.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index afec84088471..09751a5f028c 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -15,6 +15,12 @@
>   #define XATTR_CEPH_PREFIX "ceph."
>   #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
>   
> +/*
> + * Maximum size of xattrs the MDS can handle per inode by default.  This
> + * includes the attribute name and 4+4 bytes for the key/value sizes.
> + */
> +#define MDS_MAX_XATTR_PAIRS_SIZE (1<<16) /* 64K */

The max size is changeable in MDS side. Possibly we should do something 
as mentioned in your ceph PR [1].

@Jeff, any better idea ?


[1] 
https://github.com/ceph/ceph/pull/46357/commits/741f8ba36f14774834c0d5618519425ccf1ccc85#r878966753

Thanks.

-- Xiubo


> +
>   static int __remove_xattr(struct ceph_inode_info *ci,
>   			  struct ceph_inode_xattr *xattr);
>   
> @@ -1078,7 +1084,7 @@ static int ceph_sync_setxattr(struct inode *inode, const char *name,
>   			flags |= CEPH_XATTR_REMOVE;
>   	}
>   
> -	dout("setxattr value=%.*s\n", (int)size, value);
> +	dout("setxattr value size: ld\n", size);
>   
>   	/* do request */
>   	req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
> @@ -1176,8 +1182,13 @@ int __ceph_setxattr(struct inode *inode, const char *name,
>   	spin_lock(&ci->i_ceph_lock);
>   retry:
>   	issued = __ceph_caps_issued(ci, NULL);
> -	if (ci->i_xattrs.version == 0 || !(issued & CEPH_CAP_XATTR_EXCL))
> +	required_blob_size = __get_required_blob_size(ci, name_len, val_len);
> +	if ((ci->i_xattrs.version == 0) || !(issued & CEPH_CAP_XATTR_EXCL) ||
> +	    (required_blob_size >= MDS_MAX_XATTR_PAIRS_SIZE)) {
> +		dout("%s do sync setxattr: version: %llu blob size: %d\n",
> +		     __func__, ci->i_xattrs.version, required_blob_size);
>   		goto do_sync;
> +	}
>   
>   	if (!lock_snap_rwsem && !ci->i_head_snapc) {
>   		lock_snap_rwsem = true;
> @@ -1193,8 +1204,6 @@ int __ceph_setxattr(struct inode *inode, const char *name,
>   	     ceph_cap_string(issued));
>   	__build_xattrs(inode);
>   
> -	required_blob_size = __get_required_blob_size(ci, name_len, val_len);
> -
>   	if (!ci->i_xattrs.prealloc_blob ||
>   	    required_blob_size > ci->i_xattrs.prealloc_blob->alloc_len) {
>   		struct ceph_buffer *blob;
>

Re: [RFC PATCH] ceph: try to prevent exceeding the MDS maximum xattr size

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> writes:

> On 5/20/22 7:54 PM, Luís Henriques wrote:
>> The MDS tries to enforce a limit on the total key/values in extended
>> attributes.  However, this limit is enforced only if doing a synchronous
>> operation (MDS_OP_SETXATTR) -- if we're buffering the xattrs, the MDS
>> doesn't have a chance to enforce these limits.
>>
>> This patch forces the usage of the synchronous operation if xattrs size hits
>> the maximum size that is set on the MDS by default (64k).
>>
>> While there, fix a dout() that would trigger a printk warning:
>>
>> [   98.718078] ------------[ cut here ]------------
>> [   98.719012] precision 65536 too large
>> [   98.719039] WARNING: CPU: 1 PID: 3755 at lib/vsprintf.c:2703 vsnprintf+0x5e3/0x600
>> ...
>>
>> URL: https://tracker.ceph.com/issues/55725
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>>   fs/ceph/xattr.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
>> index afec84088471..09751a5f028c 100644
>> --- a/fs/ceph/xattr.c
>> +++ b/fs/ceph/xattr.c
>> @@ -15,6 +15,12 @@
>>   #define XATTR_CEPH_PREFIX "ceph."
>>   #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
>>   +/*
>> + * Maximum size of xattrs the MDS can handle per inode by default.  This
>> + * includes the attribute name and 4+4 bytes for the key/value sizes.
>> + */
>> +#define MDS_MAX_XATTR_PAIRS_SIZE (1<<16) /* 64K */
>
> The max size is changeable in MDS side. Possibly we should do something as
> mentioned in your ceph PR [1].
>

Right, as I mentioned in that PR, having clients that are aware of that
limit will allow them to behave correctly.  But the MDS still needs to
handle the case where these limits are exceeded (for ex. kernel clients
that don't know about this limit).  So, we still need to do something
similar to what I've proposed in there.

Cheers
-- 
Luís

> @Jeff, any better idea ?
>
>
> [1]
> https://github.com/ceph/ceph/pull/46357/commits/741f8ba36f14774834c0d5618519425ccf1ccc85#r878966753
>
> Thanks.
>
> -- Xiubo
>
>
>> +
>>   static int __remove_xattr(struct ceph_inode_info *ci,
>>   			  struct ceph_inode_xattr *xattr);
>>   @@ -1078,7 +1084,7 @@ static int ceph_sync_setxattr(struct inode *inode,
>> const char *name,
>>   			flags |= CEPH_XATTR_REMOVE;
>>   	}
>>   -	dout("setxattr value=%.*s\n", (int)size, value);
>> +	dout("setxattr value size: ld\n", size);
>>     	/* do request */
>>   	req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
>> @@ -1176,8 +1182,13 @@ int __ceph_setxattr(struct inode *inode, const char *name,
>>   	spin_lock(&ci->i_ceph_lock);
>>   retry:
>>   	issued = __ceph_caps_issued(ci, NULL);
>> -	if (ci->i_xattrs.version == 0 || !(issued & CEPH_CAP_XATTR_EXCL))
>> +	required_blob_size = __get_required_blob_size(ci, name_len, val_len);
>> +	if ((ci->i_xattrs.version == 0) || !(issued & CEPH_CAP_XATTR_EXCL) ||
>> +	    (required_blob_size >= MDS_MAX_XATTR_PAIRS_SIZE)) {
>> +		dout("%s do sync setxattr: version: %llu blob size: %d\n",
>> +		     __func__, ci->i_xattrs.version, required_blob_size);
>>   		goto do_sync;
>> +	}
>>     	if (!lock_snap_rwsem && !ci->i_head_snapc) {
>>   		lock_snap_rwsem = true;
>> @@ -1193,8 +1204,6 @@ int __ceph_setxattr(struct inode *inode, const char *name,
>>   	     ceph_cap_string(issued));
>>   	__build_xattrs(inode);
>>   -	required_blob_size = __get_required_blob_size(ci, name_len, val_len);
>> -
>>   	if (!ci->i_xattrs.prealloc_blob ||
>>   	    required_blob_size > ci->i_xattrs.prealloc_blob->alloc_len) {
>>   		struct ceph_buffer *blob;
>>
>

Re: [RFC PATCH] ceph: try to prevent exceeding the MDS maximum xattr size

[Jeff Layton]

On Mon, 2022-05-23 at 09:47 +0800, Xiubo Li wrote:
> On 5/20/22 7:54 PM, Luís Henriques wrote:
> > The MDS tries to enforce a limit on the total key/values in extended
> > attributes.  However, this limit is enforced only if doing a synchronous
> > operation (MDS_OP_SETXATTR) -- if we're buffering the xattrs, the MDS
> > doesn't have a chance to enforce these limits.
> > 
> > This patch forces the usage of the synchronous operation if xattrs size hits
> > the maximum size that is set on the MDS by default (64k).
> > 
> > While there, fix a dout() that would trigger a printk warning:
> > 
> > [   98.718078] ------------[ cut here ]------------
> > [   98.719012] precision 65536 too large
> > [   98.719039] WARNING: CPU: 1 PID: 3755 at lib/vsprintf.c:2703 vsnprintf+0x5e3/0x600
> > ...
> > 
> > URL: https://tracker.ceph.com/issues/55725
> > Signed-off-by: Luís Henriques <lhenriques@suse.de>
> > ---
> >   fs/ceph/xattr.c | 17 +++++++++++++----
> >   1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > index afec84088471..09751a5f028c 100644
> > --- a/fs/ceph/xattr.c
> > +++ b/fs/ceph/xattr.c
> > @@ -15,6 +15,12 @@
> >   #define XATTR_CEPH_PREFIX "ceph."
> >   #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
> >   
> > +/*
> > + * Maximum size of xattrs the MDS can handle per inode by default.  This
> > + * includes the attribute name and 4+4 bytes for the key/value sizes.
> > + */
> > +#define MDS_MAX_XATTR_PAIRS_SIZE (1<<16) /* 64K */
> 
> The max size is changeable in MDS side. Possibly we should do something 
> as mentioned in your ceph PR [1].
> 
> @Jeff, any better idea ?
> 
> 
> [1] 
> https://github.com/ceph/ceph/pull/46357/commits/741f8ba36f14774834c0d5618519425ccf1ccc85#r878966753
> 
> Thanks.
> 
> -- Xiubo
> 
> 

Not really.

The idea in the github comment makes sense. Basically, make it so that
the setting isn't changeable at runtime and then have the client query
for the limit at appropriate times.

You can probably still defeat that by changing it and rebooting the MDS,
but I don't see that you can do much else.

> > +
> >   static int __remove_xattr(struct ceph_inode_info *ci,
> >   			  struct ceph_inode_xattr *xattr);
> >   
> > @@ -1078,7 +1084,7 @@ static int ceph_sync_setxattr(struct inode *inode, const char *name,
> >   			flags |= CEPH_XATTR_REMOVE;
> >   	}
> >   
> > -	dout("setxattr value=%.*s\n", (int)size, value);
> > +	dout("setxattr value size: ld\n", size);
> >   
> >   	/* do request */
> >   	req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
> > @@ -1176,8 +1182,13 @@ int __ceph_setxattr(struct inode *inode, const char *name,
> >   	spin_lock(&ci->i_ceph_lock);
> >   retry:
> >   	issued = __ceph_caps_issued(ci, NULL);
> > -	if (ci->i_xattrs.version == 0 || !(issued & CEPH_CAP_XATTR_EXCL))
> > +	required_blob_size = __get_required_blob_size(ci, name_len, val_len);
> > +	if ((ci->i_xattrs.version == 0) || !(issued & CEPH_CAP_XATTR_EXCL) ||
> > +	    (required_blob_size >= MDS_MAX_XATTR_PAIRS_SIZE)) {
> > +		dout("%s do sync setxattr: version: %llu blob size: %d\n",
> > +		     __func__, ci->i_xattrs.version, required_blob_size);
> >   		goto do_sync;
> > +	}
> >   
> >   	if (!lock_snap_rwsem && !ci->i_head_snapc) {
> >   		lock_snap_rwsem = true;
> > @@ -1193,8 +1204,6 @@ int __ceph_setxattr(struct inode *inode, const char *name,
> >   	     ceph_cap_string(issued));
> >   	__build_xattrs(inode);
> >   
> > -	required_blob_size = __get_required_blob_size(ci, name_len, val_len);
> > -
> >   	if (!ci->i_xattrs.prealloc_blob ||
> >   	    required_blob_size > ci->i_xattrs.prealloc_blob->alloc_len) {
> >   		struct ceph_buffer *blob;
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH] ceph: try to prevent exceeding the MDS maximum xattr size

[Xiubo Li]

On 5/23/22 6:43 PM, Jeff Layton wrote:
> On Mon, 2022-05-23 at 09:47 +0800, Xiubo Li wrote:
>> On 5/20/22 7:54 PM, Luís Henriques wrote:
>>> The MDS tries to enforce a limit on the total key/values in extended
>>> attributes.  However, this limit is enforced only if doing a synchronous
>>> operation (MDS_OP_SETXATTR) -- if we're buffering the xattrs, the MDS
>>> doesn't have a chance to enforce these limits.
>>>
>>> This patch forces the usage of the synchronous operation if xattrs size hits
>>> the maximum size that is set on the MDS by default (64k).
>>>
>>> While there, fix a dout() that would trigger a printk warning:
>>>
>>> [   98.718078] ------------[ cut here ]------------
>>> [   98.719012] precision 65536 too large
>>> [   98.719039] WARNING: CPU: 1 PID: 3755 at lib/vsprintf.c:2703 vsnprintf+0x5e3/0x600
>>> ...
>>>
>>> URL: https://tracker.ceph.com/issues/55725
>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>> ---
>>>    fs/ceph/xattr.c | 17 +++++++++++++----
>>>    1 file changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
>>> index afec84088471..09751a5f028c 100644
>>> --- a/fs/ceph/xattr.c
>>> +++ b/fs/ceph/xattr.c
>>> @@ -15,6 +15,12 @@
>>>    #define XATTR_CEPH_PREFIX "ceph."
>>>    #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
>>>    
>>> +/*
>>> + * Maximum size of xattrs the MDS can handle per inode by default.  This
>>> + * includes the attribute name and 4+4 bytes for the key/value sizes.
>>> + */
>>> +#define MDS_MAX_XATTR_PAIRS_SIZE (1<<16) /* 64K */
>> The max size is changeable in MDS side. Possibly we should do something
>> as mentioned in your ceph PR [1].
>>
>> @Jeff, any better idea ?
>>
>>
>> [1]
>> https://github.com/ceph/ceph/pull/46357/commits/741f8ba36f14774834c0d5618519425ccf1ccc85#r878966753
>>
>> Thanks.
>>
>> -- Xiubo
>>
>>
> Not really.
>
> The idea in the github comment makes sense. Basically, make it so that
> the setting isn't changeable at runtime and then have the client query
> for the limit at appropriate times.
>
> You can probably still defeat that by changing it and rebooting the MDS,
> but I don't see that you can do much else.

Yeah, sounds good.


>
>>> +
>>>    static int __remove_xattr(struct ceph_inode_info *ci,
>>>    			  struct ceph_inode_xattr *xattr);
>>>    
>>> @@ -1078,7 +1084,7 @@ static int ceph_sync_setxattr(struct inode *inode, const char *name,
>>>    			flags |= CEPH_XATTR_REMOVE;
>>>    	}
>>>    
>>> -	dout("setxattr value=%.*s\n", (int)size, value);
>>> +	dout("setxattr value size: ld\n", size);
>>>    
>>>    	/* do request */
>>>    	req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
>>> @@ -1176,8 +1182,13 @@ int __ceph_setxattr(struct inode *inode, const char *name,
>>>    	spin_lock(&ci->i_ceph_lock);
>>>    retry:
>>>    	issued = __ceph_caps_issued(ci, NULL);
>>> -	if (ci->i_xattrs.version == 0 || !(issued & CEPH_CAP_XATTR_EXCL))
>>> +	required_blob_size = __get_required_blob_size(ci, name_len, val_len);
>>> +	if ((ci->i_xattrs.version == 0) || !(issued & CEPH_CAP_XATTR_EXCL) ||
>>> +	    (required_blob_size >= MDS_MAX_XATTR_PAIRS_SIZE)) {
>>> +		dout("%s do sync setxattr: version: %llu blob size: %d\n",
>>> +		     __func__, ci->i_xattrs.version, required_blob_size);
>>>    		goto do_sync;
>>> +	}
>>>    
>>>    	if (!lock_snap_rwsem && !ci->i_head_snapc) {
>>>    		lock_snap_rwsem = true;
>>> @@ -1193,8 +1204,6 @@ int __ceph_setxattr(struct inode *inode, const char *name,
>>>    	     ceph_cap_string(issued));
>>>    	__build_xattrs(inode);
>>>    
>>> -	required_blob_size = __get_required_blob_size(ci, name_len, val_len);
>>> -
>>>    	if (!ci->i_xattrs.prealloc_blob ||
>>>    	    required_blob_size > ci->i_xattrs.prealloc_blob->alloc_len) {
>>>    		struct ceph_buffer *blob;
>>>