Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 24 Nov 2020 08:37:02 +1100
From:   Dave Chinner <david@fromorbit.com>
To:     "Darrick J. Wong" <darrick.wong@oracle.com>
Cc:     XiaoLi Feng <xifeng@redhat.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        linux-kernel@vger.kernel.org, ira.weiny@intel.com,
        Xiaoli Feng <fengxiaoli0714@gmail.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH] fs/stat: set attributes_mask for STATX_ATTR_DAX
Message-ID: <20201123213702.GV7391@dread.disaster.area>
References: <20201121003331.21342-1-xifeng@redhat.com>
 <21890103-fce2-bb50-7fc2-6c6d509b982f@infradead.org>
 <20201121011516.GD3837269@magnolia>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201121011516.GD3837269@magnolia>
Precedence: bulk

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