* Re: [PATCH] fs/stat: set attributes_mask for STATX_ATTR_DAX
2020-11-21 2:03 ` [PATCH] fs/stat: set attributes_mask for STATX_ATTR_DAX Darrick J. Wong
@ 2020-11-22 15:57 ` Xiaoli Feng
2020-11-23 21:37 ` Dave Chinner
1 sibling, 0 replies; 3+ messages in thread
From: Xiaoli Feng @ 2020-11-22 15:57 UTC (permalink / raw)
To: Darrick J. Wong, Eric Sandeen
Cc: Randy Dunlap, linux-kernel, ira weiny, Xiaoli Feng, linux-fsdevel
Hi,
Thanks for review.
----- Original Message -----
> From: "Darrick J. Wong" <darrick.wong@oracle.com>
> To: "XiaoLi Feng" <xifeng@redhat.com>
> Cc: "Randy Dunlap" <rdunlap@infradead.org>, linux-kernel@vger.kernel.org, "ira weiny" <ira.weiny@intel.com>, "Xiaoli
> Feng" <fengxiaoli0714@gmail.com>, "linux-fsdevel" <linux-fsdevel@vger.kernel.org>
> Sent: Saturday, November 21, 2020 10:03:18 AM
> Subject: Re: [PATCH] fs/stat: set attributes_mask for STATX_ATTR_DAX
>
> [Adding fsdevel to cc since this is a filesystems question]
>
> On Fri, Nov 20, 2020 at 04:58:09PM -0800, Randy Dunlap wrote:
> > Hi,
> >
> > I don't know this code, but:
> >
> > On 11/20/20 4:33 PM, XiaoLi Feng wrote:
> > > From: Xiaoli Feng <fengxiaoli0714@gmail.com>
> > >
> > > keep attributes and attributes_mask are consistent for
> > > STATX_ATTR_DAX.
> > > ---
> > > fs/stat.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/stat.c b/fs/stat.c
> > > index dacecdda2e79..914a61d256b0 100644
> > > --- a/fs/stat.c
> > > +++ b/fs/stat.c
> > > @@ -82,7 +82,7 @@ int vfs_getattr_nosec(const struct path *path, struct
> > > kstat *stat,
> > >
> > > if (IS_DAX(inode))
> > > stat->attributes |= STATX_ATTR_DAX;
> > > -
> > > + stat->attributes_mask |= STATX_ATTR_DAX;
> >
> > Why shouldn't that be:
> >
> > if (IS_DAX(inode))
> > stat->attributes_mask |= STATX_ATTR_DAX;
> >
> > or combine them, like this:
> >
> > if (IS_DAX(inode)) {
> > stat->attributes |= STATX_ATTR_DAX;
> > stat->attributes_mask |= STATX_ATTR_DAX;
> > }
> >
> >
> > and no need to delete that blank line.
>
> Some filesystems could support DAX but not have it enabled for this
> particular file, so this won't work.
>
> General question: should filesystems that are /capable/ of DAX signal
> this by setting the DAX bit in the attributes mask? Or is this a VFS
> feature and hence here is the appropriate place to be setting the mask?
Actually I just see here set the attributes. Then set the attributes mask
after it.
>
> Extra question: should we only set this in the attributes mask if
> CONFIG_FS_DAX=y ?
No, my origin patch always set this attributes mask. It's out of if condition.
if (IS_DAX(inode))
stat->attributes |= STATX_ATTR_DAX;
-
+ stat->attributes_mask |= STATX_ATTR_DAX;
>
> --D
>
> > > if (inode->i_op->getattr)
> > > return inode->i_op->getattr(path, stat, request_mask,
> > > query_flags);
> > >
> >
> > thanks.
> > --
> > ~Randy
> >
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] fs/stat: set attributes_mask for STATX_ATTR_DAX
2020-11-21 2:03 ` [PATCH] fs/stat: set attributes_mask for STATX_ATTR_DAX Darrick J. Wong
2020-11-22 15:57 ` Xiaoli Feng
@ 2020-11-23 21:37 ` Dave Chinner
1 sibling, 0 replies; 3+ messages in thread
From: Dave Chinner @ 2020-11-23 21:37 UTC (permalink / raw)
To: Darrick J. Wong
Cc: XiaoLi Feng, Randy Dunlap, linux-kernel, ira.weiny, Xiaoli Feng,
linux-fsdevel
On Fri, Nov 20, 2020 at 06:03:18PM -0800, Darrick J. Wong wrote:
> [Adding fsdevel to cc since this is a filesystems question]
>
> On Fri, Nov 20, 2020 at 04:58:09PM -0800, Randy Dunlap wrote:
> > Hi,
> >
> > I don't know this code, but:
> >
> > On 11/20/20 4:33 PM, XiaoLi Feng wrote:
> > > From: Xiaoli Feng <fengxiaoli0714@gmail.com>
> > >
> > > keep attributes and attributes_mask are consistent for
> > > STATX_ATTR_DAX.
> > > ---
> > > fs/stat.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/stat.c b/fs/stat.c
> > > index dacecdda2e79..914a61d256b0 100644
> > > --- a/fs/stat.c
> > > +++ b/fs/stat.c
> > > @@ -82,7 +82,7 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
> > >
> > > if (IS_DAX(inode))
> > > stat->attributes |= STATX_ATTR_DAX;
> > > -
> > > + stat->attributes_mask |= STATX_ATTR_DAX;
> >
> > Why shouldn't that be:
> >
> > if (IS_DAX(inode))
> > stat->attributes_mask |= STATX_ATTR_DAX;
> >
> > or combine them, like this:
> >
> > if (IS_DAX(inode)) {
> > stat->attributes |= STATX_ATTR_DAX;
> > stat->attributes_mask |= STATX_ATTR_DAX;
> > }
> >
> >
> > and no need to delete that blank line.
>
> Some filesystems could support DAX but not have it enabled for this
> particular file, so this won't work.
>
> General question: should filesystems that are /capable/ of DAX signal
> this by setting the DAX bit in the attributes mask?
I think so, yes. It could be set if the right bit on the inode is
set, but it currently isn't so the bit in the mask is set but the
bit in the attributes is not. i.e "DAX is valid status bit, but it
is not set for this file".
> Or is this a VFS
> feature and hence here is the appropriate place to be setting the mask?
Well, in the end it's a filesystem feature bit because the
filesystem policy that decides whether DAX is used or not. e.g. if
the block device is not DAX capable or dax=never mount option is
set, we should not ever set STATX_ATTR_DAX in statx for either the
attributes or attributes_mask field because the filesystem is not
DAX capable. And given that we have filesystems with multiple block
devices that can have different DAX capabilities, I think this
statx() attr state (and mask) really has to come from the
filesystem, not VFS...
> Extra question: should we only set this in the attributes mask if
> CONFIG_FS_DAX=y ?
IMO, yes, because it will always be false on CONFIG_FS_DAX=n and so
it may well as not be emitted as a supported bit in the mask.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 3+ messages in thread