[PATCH v2] ceph: don't check for quotas on MDS stray dirs

[PATCH v2] ceph: don't check for quotas on MDS stray dirs

[Jeff Layton]

玮文 胡 reported seeing the WARN_RATELIMIT pop when writing to an
inode that had been transplanted into the stray dir. The client was
trying to look up the quotarealm info from the parent and that tripped
the warning.

Change the ceph_vino_is_reserved helper to not throw a warning for
MDS stray directories (0x100 - 0x1ff), only for reserved dirs that
are not in that range.

Also, fix ceph_has_realms_with_quotas to return false when encountering
a reserved inode.

URL: https://tracker.ceph.com/issues/53180
Reported-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
Reviewed-by: Luis Henriques <lhenriques@suse.de>
Reviewed-by: Xiubo Li <xiubli@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/quota.c |  3 +++
 fs/ceph/super.h | 20 ++++++++++++--------
 2 files changed, 15 insertions(+), 8 deletions(-)

I was still seeing some warnings even with the earlier patch, so I
decided to rework it to just never warn on MDS stray dirs. This should
also silence the warnings on MDS stray dirs (and also alleviate Luis'
concern about the function renaming in the earlier patch).

diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c
index 24ae13ea2241..a338a3ec0dc4 100644
--- a/fs/ceph/quota.c
+++ b/fs/ceph/quota.c
@@ -30,6 +30,9 @@ static inline bool ceph_has_realms_with_quotas(struct inode *inode)
 	/* if root is the real CephFS root, we don't have quota realms */
 	if (root && ceph_ino(root) == CEPH_INO_ROOT)
 		return false;
+	/* MDS stray dirs have no quota realms */
+	if (ceph_vino_is_reserved(ceph_inode(inode)->i_vino))
+		return false;
 	/* otherwise, we can't know for sure */
 	return true;
 }
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 387ee33894db..f9b1bbf26c1b 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -541,19 +541,23 @@ static inline int ceph_ino_compare(struct inode *inode, void *data)
  *
  * These come from src/mds/mdstypes.h in the ceph sources.
  */
-#define CEPH_MAX_MDS		0x100
-#define CEPH_NUM_STRAY		10
+#define CEPH_MAX_MDS			0x100
+#define CEPH_NUM_STRAY			10
 #define CEPH_MDS_INO_MDSDIR_OFFSET	(1 * CEPH_MAX_MDS)
+#define CEPH_MDS_INO_LOG_OFFSET		(2 * CEPH_MAX_MDS)
 #define CEPH_INO_SYSTEM_BASE		((6*CEPH_MAX_MDS) + (CEPH_MAX_MDS * CEPH_NUM_STRAY))
 
 static inline bool ceph_vino_is_reserved(const struct ceph_vino vino)
 {
-	if (vino.ino < CEPH_INO_SYSTEM_BASE &&
-	    vino.ino >= CEPH_MDS_INO_MDSDIR_OFFSET) {
-		WARN_RATELIMIT(1, "Attempt to access reserved inode number 0x%llx", vino.ino);
-		return true;
-	}
-	return false;
+	if (vino.ino >= CEPH_INO_SYSTEM_BASE ||
+	    vino.ino < CEPH_MDS_INO_MDSDIR_OFFSET)
+		return false;
+
+	/* Don't warn on mdsdirs */
+	WARN_RATELIMIT(vino.ino >= CEPH_MDS_INO_LOG_OFFSET,
+			"Attempt to access reserved inode number 0x%llx",
+			vino.ino);
+	return true;
 }
 
 static inline struct inode *ceph_find_inode(struct super_block *sb,
-- 
2.33.1

Re: [PATCH v2] ceph: don't check for quotas on MDS stray dirs

[Jeff Layton]

On Tue, 2021-12-07 at 10:27 -0500, Jeff Layton wrote:
> 玮文 胡 reported seeing the WARN_RATELIMIT pop when writing to an
> inode that had been transplanted into the stray dir. The client was
> trying to look up the quotarealm info from the parent and that tripped
> the warning.
> 
> Change the ceph_vino_is_reserved helper to not throw a warning for
> MDS stray directories (0x100 - 0x1ff), only for reserved dirs that
> are not in that range.
> 
> Also, fix ceph_has_realms_with_quotas to return false when encountering
> a reserved inode.
> 
> URL: https://tracker.ceph.com/issues/53180
> Reported-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> Reviewed-by: Luis Henriques <lhenriques@suse.de>
> Reviewed-by: Xiubo Li <xiubli@redhat.com>

Oops, I forgot to remote the Reviewed-by: lines before sending since
this patch is different. The one in the testing branch has them removed.
Reviews of this patch would still be welcome.

> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/quota.c |  3 +++
>  fs/ceph/super.h | 20 ++++++++++++--------
>  2 files changed, 15 insertions(+), 8 deletions(-)
> 
> I was still seeing some warnings even with the earlier patch, so I
> decided to rework it to just never warn on MDS stray dirs. This should
> also silence the warnings on MDS stray dirs (and also alleviate Luis'
> concern about the function renaming in the earlier patch).
> 
> diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c
> index 24ae13ea2241..a338a3ec0dc4 100644
> --- a/fs/ceph/quota.c
> +++ b/fs/ceph/quota.c
> @@ -30,6 +30,9 @@ static inline bool ceph_has_realms_with_quotas(struct inode *inode)
>  	/* if root is the real CephFS root, we don't have quota realms */
>  	if (root && ceph_ino(root) == CEPH_INO_ROOT)
>  		return false;
> +	/* MDS stray dirs have no quota realms */
> +	if (ceph_vino_is_reserved(ceph_inode(inode)->i_vino))
> +		return false;
>  	/* otherwise, we can't know for sure */
>  	return true;
>  }
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 387ee33894db..f9b1bbf26c1b 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -541,19 +541,23 @@ static inline int ceph_ino_compare(struct inode *inode, void *data)
>   *
>   * These come from src/mds/mdstypes.h in the ceph sources.
>   */
> -#define CEPH_MAX_MDS		0x100
> -#define CEPH_NUM_STRAY		10
> +#define CEPH_MAX_MDS			0x100
> +#define CEPH_NUM_STRAY			10
>  #define CEPH_MDS_INO_MDSDIR_OFFSET	(1 * CEPH_MAX_MDS)
> +#define CEPH_MDS_INO_LOG_OFFSET		(2 * CEPH_MAX_MDS)
>  #define CEPH_INO_SYSTEM_BASE		((6*CEPH_MAX_MDS) + (CEPH_MAX_MDS * CEPH_NUM_STRAY))
>  
>  static inline bool ceph_vino_is_reserved(const struct ceph_vino vino)
>  {
> -	if (vino.ino < CEPH_INO_SYSTEM_BASE &&
> -	    vino.ino >= CEPH_MDS_INO_MDSDIR_OFFSET) {
> -		WARN_RATELIMIT(1, "Attempt to access reserved inode number 0x%llx", vino.ino);
> -		return true;
> -	}
> -	return false;
> +	if (vino.ino >= CEPH_INO_SYSTEM_BASE ||
> +	    vino.ino < CEPH_MDS_INO_MDSDIR_OFFSET)
> +		return false;
> +
> +	/* Don't warn on mdsdirs */
> +	WARN_RATELIMIT(vino.ino >= CEPH_MDS_INO_LOG_OFFSET,
> +			"Attempt to access reserved inode number 0x%llx",
> +			vino.ino);
> +	return true;
>  }
>  
>  static inline struct inode *ceph_find_inode(struct super_block *sb,

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] ceph: don't check for quotas on MDS stray dirs

[Luís Henriques]

On Tue, Dec 07, 2021 at 10:29:28AM -0500, Jeff Layton wrote:
> On Tue, 2021-12-07 at 10:27 -0500, Jeff Layton wrote:
> > 玮文 胡 reported seeing the WARN_RATELIMIT pop when writing to an
> > inode that had been transplanted into the stray dir. The client was
> > trying to look up the quotarealm info from the parent and that tripped
> > the warning.
> > 
> > Change the ceph_vino_is_reserved helper to not throw a warning for
> > MDS stray directories (0x100 - 0x1ff), only for reserved dirs that
> > are not in that range.
> > 
> > Also, fix ceph_has_realms_with_quotas to return false when encountering
> > a reserved inode.
> > 
> > URL: https://tracker.ceph.com/issues/53180
> > Reported-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> > Reviewed-by: Luis Henriques <lhenriques@suse.de>
> > Reviewed-by: Xiubo Li <xiubli@redhat.com>
> 
> Oops, I forgot to remote the Reviewed-by: lines before sending since
> this patch is different. The one in the testing branch has them removed.
> Reviews of this patch would still be welcome.

Feel free to add my Reviewed-by: back, the patch looks OK to me.  Thanks!

Cheers,
--
Luís

> 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/quota.c |  3 +++
> >  fs/ceph/super.h | 20 ++++++++++++--------
> >  2 files changed, 15 insertions(+), 8 deletions(-)
> > 
> > I was still seeing some warnings even with the earlier patch, so I
> > decided to rework it to just never warn on MDS stray dirs. This should
> > also silence the warnings on MDS stray dirs (and also alleviate Luis'
> > concern about the function renaming in the earlier patch).
> > 
> > diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c
> > index 24ae13ea2241..a338a3ec0dc4 100644
> > --- a/fs/ceph/quota.c
> > +++ b/fs/ceph/quota.c
> > @@ -30,6 +30,9 @@ static inline bool ceph_has_realms_with_quotas(struct inode *inode)
> >  	/* if root is the real CephFS root, we don't have quota realms */
> >  	if (root && ceph_ino(root) == CEPH_INO_ROOT)
> >  		return false;
> > +	/* MDS stray dirs have no quota realms */
> > +	if (ceph_vino_is_reserved(ceph_inode(inode)->i_vino))
> > +		return false;
> >  	/* otherwise, we can't know for sure */
> >  	return true;
> >  }
> > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > index 387ee33894db..f9b1bbf26c1b 100644
> > --- a/fs/ceph/super.h
> > +++ b/fs/ceph/super.h
> > @@ -541,19 +541,23 @@ static inline int ceph_ino_compare(struct inode *inode, void *data)
> >   *
> >   * These come from src/mds/mdstypes.h in the ceph sources.
> >   */
> > -#define CEPH_MAX_MDS		0x100
> > -#define CEPH_NUM_STRAY		10
> > +#define CEPH_MAX_MDS			0x100
> > +#define CEPH_NUM_STRAY			10
> >  #define CEPH_MDS_INO_MDSDIR_OFFSET	(1 * CEPH_MAX_MDS)
> > +#define CEPH_MDS_INO_LOG_OFFSET		(2 * CEPH_MAX_MDS)
> >  #define CEPH_INO_SYSTEM_BASE		((6*CEPH_MAX_MDS) + (CEPH_MAX_MDS * CEPH_NUM_STRAY))
> >  
> >  static inline bool ceph_vino_is_reserved(const struct ceph_vino vino)
> >  {
> > -	if (vino.ino < CEPH_INO_SYSTEM_BASE &&
> > -	    vino.ino >= CEPH_MDS_INO_MDSDIR_OFFSET) {
> > -		WARN_RATELIMIT(1, "Attempt to access reserved inode number 0x%llx", vino.ino);
> > -		return true;
> > -	}
> > -	return false;
> > +	if (vino.ino >= CEPH_INO_SYSTEM_BASE ||
> > +	    vino.ino < CEPH_MDS_INO_MDSDIR_OFFSET)
> > +		return false;
> > +
> > +	/* Don't warn on mdsdirs */
> > +	WARN_RATELIMIT(vino.ino >= CEPH_MDS_INO_LOG_OFFSET,
> > +			"Attempt to access reserved inode number 0x%llx",
> > +			vino.ino);
> > +	return true;
> >  }
> >  
> >  static inline struct inode *ceph_find_inode(struct super_block *sb,
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] ceph: don't check for quotas on MDS stray dirs

[Xiubo Li]

On 12/7/21 11:27 PM, Jeff Layton wrote:
> 玮文 胡 reported seeing the WARN_RATELIMIT pop when writing to an
> inode that had been transplanted into the stray dir. The client was
> trying to look up the quotarealm info from the parent and that tripped
> the warning.
>
> Change the ceph_vino_is_reserved helper to not throw a warning for
> MDS stray directories (0x100 - 0x1ff), only for reserved dirs that
> are not in that range.
>
> Also, fix ceph_has_realms_with_quotas to return false when encountering
> a reserved inode.
>
> URL: https://tracker.ceph.com/issues/53180
> Reported-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> Reviewed-by: Luis Henriques <lhenriques@suse.de>
> Reviewed-by: Xiubo Li <xiubli@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/ceph/quota.c |  3 +++
>   fs/ceph/super.h | 20 ++++++++++++--------
>   2 files changed, 15 insertions(+), 8 deletions(-)
>
> I was still seeing some warnings even with the earlier patch, so I
> decided to rework it to just never warn on MDS stray dirs. This should
> also silence the warnings on MDS stray dirs (and also alleviate Luis'
> concern about the function renaming in the earlier patch).
>
> diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c
> index 24ae13ea2241..a338a3ec0dc4 100644
> --- a/fs/ceph/quota.c
> +++ b/fs/ceph/quota.c
> @@ -30,6 +30,9 @@ static inline bool ceph_has_realms_with_quotas(struct inode *inode)
>   	/* if root is the real CephFS root, we don't have quota realms */
>   	if (root && ceph_ino(root) == CEPH_INO_ROOT)
>   		return false;
> +	/* MDS stray dirs have no quota realms */
> +	if (ceph_vino_is_reserved(ceph_inode(inode)->i_vino))
> +		return false;
>   	/* otherwise, we can't know for sure */
>   	return true;
>   }
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 387ee33894db..f9b1bbf26c1b 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -541,19 +541,23 @@ static inline int ceph_ino_compare(struct inode *inode, void *data)
>    *
>    * These come from src/mds/mdstypes.h in the ceph sources.
>    */
> -#define CEPH_MAX_MDS		0x100
> -#define CEPH_NUM_STRAY		10
> +#define CEPH_MAX_MDS			0x100
> +#define CEPH_NUM_STRAY			10
>   #define CEPH_MDS_INO_MDSDIR_OFFSET	(1 * CEPH_MAX_MDS)
> +#define CEPH_MDS_INO_LOG_OFFSET		(2 * CEPH_MAX_MDS)
>   #define CEPH_INO_SYSTEM_BASE		((6*CEPH_MAX_MDS) + (CEPH_MAX_MDS * CEPH_NUM_STRAY))
>   
>   static inline bool ceph_vino_is_reserved(const struct ceph_vino vino)
>   {
> -	if (vino.ino < CEPH_INO_SYSTEM_BASE &&
> -	    vino.ino >= CEPH_MDS_INO_MDSDIR_OFFSET) {
> -		WARN_RATELIMIT(1, "Attempt to access reserved inode number 0x%llx", vino.ino);
> -		return true;
> -	}
> -	return false;
> +	if (vino.ino >= CEPH_INO_SYSTEM_BASE ||
> +	    vino.ino < CEPH_MDS_INO_MDSDIR_OFFSET)
> +		return false;
> +
> +	/* Don't warn on mdsdirs */
> +	WARN_RATELIMIT(vino.ino >= CEPH_MDS_INO_LOG_OFFSET,
> +			"Attempt to access reserved inode number 0x%llx",
> +			vino.ino);
> +	return true;
>   }
>   
>   static inline struct inode *ceph_find_inode(struct super_block *sb,

LGTM.

Reviewed-by: Xiubo Li <xiubli@redhat.com>