[PATCH] xfs: fix userdata allocation detection regression

[PATCH] xfs: fix userdata allocation detection regression

[Christoph Hellwig]

The XFS_ALLOC_USERDATA was not directly used, but indirectly in the
xfs_alloc_is_userdata function that check for any bit that is not
XFS_ALLOC_NOBUSY being set.  But XFS_ALLOC_NOBUSY is equivalent to
a user data allocation, so rename that flag and check for that instead
to reduce the confusion.

Fixes: 1baa2800e62d ("xfs: remove the unused XFS_ALLOC_USERDATA flag")
Reported-by: kernel test robot <rong.a.chen@intel.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_alloc.h | 7 ++++---
 fs/xfs/libxfs/xfs_bmap.c  | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
index 58fa85cec325..24710746cecb 100644
--- a/fs/xfs/libxfs/xfs_alloc.h
+++ b/fs/xfs/libxfs/xfs_alloc.h
@@ -83,18 +83,19 @@ typedef struct xfs_alloc_arg {
  */
 #define XFS_ALLOC_INITIAL_USER_DATA	(1 << 0)/* special case start of file */
 #define XFS_ALLOC_USERDATA_ZERO		(1 << 1)/* zero extent on allocation */
-#define XFS_ALLOC_NOBUSY		(1 << 2)/* Busy extents not allowed */
+#define XFS_ALLOC_USERDATA		(1 << 2)/* allocation is for user data*/
 
 static inline bool
 xfs_alloc_is_userdata(int datatype)
 {
-	return (datatype & ~XFS_ALLOC_NOBUSY) != 0;
+	return (datatype & XFS_ALLOC_USERDATA);
 }
 
 static inline bool
 xfs_alloc_allow_busy_reuse(int datatype)
 {
-	return (datatype & XFS_ALLOC_NOBUSY) == 0;
+	/* Busy extents not allowed for user data */
+	return (datatype & XFS_ALLOC_USERDATA) == 0;
 }
 
 /* freespace limit calculations */
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 054b4ce30033..a2d8c4e4cad5 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4041,7 +4041,7 @@ xfs_bmapi_allocate(
 	 * the busy list.
 	 */
 	if (!(bma->flags & XFS_BMAPI_METADATA)) {
-		bma->datatype = XFS_ALLOC_NOBUSY;
+		bma->datatype = XFS_ALLOC_USERDATA;
 		if (whichfork == XFS_DATA_FORK && bma->offset == 0)
 			bma->datatype |= XFS_ALLOC_INITIAL_USER_DATA;
 		if (bma->flags & XFS_BMAPI_ZERO)
-- 
2.20.1

Re: [PATCH] xfs: fix userdata allocation detection regression

[Brian Foster]

On Thu, Sep 19, 2019 at 07:19:43PM -0700, Christoph Hellwig wrote:
> The XFS_ALLOC_USERDATA was not directly used, but indirectly in the
> xfs_alloc_is_userdata function that check for any bit that is not
> XFS_ALLOC_NOBUSY being set.  But XFS_ALLOC_NOBUSY is equivalent to
> a user data allocation, so rename that flag and check for that instead
> to reduce the confusion.
> 
> Fixes: 1baa2800e62d ("xfs: remove the unused XFS_ALLOC_USERDATA flag")
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_alloc.h | 7 ++++---
>  fs/xfs/libxfs/xfs_bmap.c  | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
> index 58fa85cec325..24710746cecb 100644
> --- a/fs/xfs/libxfs/xfs_alloc.h
> +++ b/fs/xfs/libxfs/xfs_alloc.h
> @@ -83,18 +83,19 @@ typedef struct xfs_alloc_arg {
>   */
>  #define XFS_ALLOC_INITIAL_USER_DATA	(1 << 0)/* special case start of file */
>  #define XFS_ALLOC_USERDATA_ZERO		(1 << 1)/* zero extent on allocation */
> -#define XFS_ALLOC_NOBUSY		(1 << 2)/* Busy extents not allowed */
> +#define XFS_ALLOC_USERDATA		(1 << 2)/* allocation is for user data*/
>  
>  static inline bool
>  xfs_alloc_is_userdata(int datatype)
>  {
> -	return (datatype & ~XFS_ALLOC_NOBUSY) != 0;
> +	return (datatype & XFS_ALLOC_USERDATA);
>  }
>  

Prior to this change (and commit 1baa2800e62d), something like an xattr
remote value block would not be considered user data. As of this change,
that is no longer the case. That seems reasonable on first thought (it
is user data after all), but I'm not so sure it's appropriate once you
look through some of the ways xfs_alloc_is_userdata() is used.

Brian

>  static inline bool
>  xfs_alloc_allow_busy_reuse(int datatype)
>  {
> -	return (datatype & XFS_ALLOC_NOBUSY) == 0;
> +	/* Busy extents not allowed for user data */
> +	return (datatype & XFS_ALLOC_USERDATA) == 0;
>  }
>  
>  /* freespace limit calculations */
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 054b4ce30033..a2d8c4e4cad5 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4041,7 +4041,7 @@ xfs_bmapi_allocate(
>  	 * the busy list.
>  	 */
>  	if (!(bma->flags & XFS_BMAPI_METADATA)) {
> -		bma->datatype = XFS_ALLOC_NOBUSY;
> +		bma->datatype = XFS_ALLOC_USERDATA;
>  		if (whichfork == XFS_DATA_FORK && bma->offset == 0)
>  			bma->datatype |= XFS_ALLOC_INITIAL_USER_DATA;
>  		if (bma->flags & XFS_BMAPI_ZERO)
> -- 
> 2.20.1
> 

Re: [PATCH] xfs: fix userdata allocation detection regression

[Christoph Hellwig]

On Mon, Sep 23, 2019 at 08:48:36AM -0400, Brian Foster wrote:
> Prior to this change (and commit 1baa2800e62d), something like an xattr
> remote value block would not be considered user data. As of this change,
> that is no longer the case. That seems reasonable on first thought (it
> is user data after all), but I'm not so sure it's appropriate once you
> look through some of the ways xfs_alloc_is_userdata() is used.

True.  Let's just revert the original patch for now, and sort out the
mess of these flags properly later.