[PATCH] fs/stat: set attributes_mask for STATX_ATTR_DAX

[PATCH] fs/stat: set attributes_mask for STATX_ATTR_DAX

[XiaoLi Feng]

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;
 	if (inode->i_op->getattr)
 		return inode->i_op->getattr(path, stat, request_mask,
 					    query_flags);
-- 
2.18.1

Re: [PATCH] fs/stat: set attributes_mask for STATX_ATTR_DAX

[Randy Dunlap]

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.

>  	if (inode->i_op->getattr)
>  		return inode->i_op->getattr(path, stat, request_mask,
>  					    query_flags);
> 

thanks.
-- 
~Randy

Re: [PATCH] fs/stat: set attributes_mask for STATX_ATTR_DAX

[Darrick J. Wong]

[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?

Extra question: should we only set this in the attributes mask if
CONFIG_FS_DAX=y ?

--D

> >  	if (inode->i_op->getattr)
> >  		return inode->i_op->getattr(path, stat, request_mask,
> >  					    query_flags);
> > 
> 
> thanks.
> -- 
> ~Randy
> 

Re: [PATCH] fs/stat: set attributes_mask for STATX_ATTR_DAX

[Xiaoli Feng]

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
> > 
> 
> 

Re: [PATCH] fs/stat: set attributes_mask for STATX_ATTR_DAX

[Dave Chinner]

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