| Message ID | 20190920021943.26930-1-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: fix userdata allocation detection regression | expand |
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 >
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.
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)
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(-)