linux-ext4 - Re: [PATCH 2 2/2] xfs: fix rt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 1 Feb 2018 16:43:32 -0800
From:   "Darrick J. Wong" <darrick.wong@...cle.com>
To:     Dave Chinner <david@...morbit.com>
Cc:     Dave Jiang <dave.jiang@...el.com>, linux-xfs@...r.kernel.org,
        ross.zwisler@...ux.intel.com, linux-ext4@...r.kernel.org,
        dan.j.williams@...el.com, linux-nvdimm@...ts.01.org
Subject: Re: [PATCH 2 2/2] xfs: fix rt_dev usage for DAX

On Fri, Feb 02, 2018 at 10:44:13AM +1100, Dave Chinner wrote:
> On Thu, Feb 01, 2018 at 01:33:05PM -0700, Dave Jiang wrote:
> > When using realtime device (rtdev) with xfs where the data device is not
> > DAX capable, two issues arise. One is when data device is not DAX but the
> > realtime device is DAX capable, we currently disable DAX.
> > After passing this check, we are also not marking the inode as DAX capable.
> > This change will allow DAX enabled if the data device or the realtime
> > device is DAX capable. S_DAX will be marked for the inode if the file is
> > residing on a DAX capable device. This will prevent the case of rtdev is not
> > DAX and data device is DAX to create realtime files.
> 
> I'm confused by this description. I'm not sure what is broken, nor
> what you are trying to fix.
> 
> I think what you want to do is enable DAX on RT devices separately
> to the data device and vice versa?
> 
> i.e. is this what you are trying to acheive?
> 
> datadev dax	rtdev dax		DAX enabled on
> -----------     ---------		--------------
> no		no			neither
> yes		no			datadev
> no		yes			rtdev
> yes		yes			both
> 
> 
> > 
> > Signed-off-by: Dave Jiang <dave.jiang@...el.com>
> > Reported-by: Darrick Wong <darrick.wong@...cle.com>
> > ---
> >  fs/xfs/xfs_iops.c  |    3 ++-
> >  fs/xfs/xfs_super.c |    9 ++++++++-
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index 56475fcd76f2..ab352c325301 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -1204,7 +1204,8 @@ xfs_diflags_to_iflags(
> >  	    ip->i_mount->m_sb.sb_blocksize == PAGE_SIZE &&
> >  	    !xfs_is_reflink_inode(ip) &&
> >  	    (ip->i_mount->m_flags & XFS_MOUNT_DAX ||
> > -	     ip->i_d.di_flags2 & XFS_DIFLAG2_DAX))
> > +	     ip->i_d.di_flags2 & XFS_DIFLAG2_DAX) &&
> > +	    blk_queue_dax(bdev_get_queue(inode->i_sb->s_bdev)))
> 
> This does not discriminate between the rtdev or the data dev. This
> needs to call xfs_find_bdev_for_inode() to get the right device
> for the inode config.
> 
> Further, if we add or remove the RT flag to the inode at a later
> point in time (e.g. via ioctl) we also need to re-evaluate the S_DAX
> flag at that point in time.

Ah, right, I'd missed that subtlety in my earlier replies.  Ok, add
another patch to this series to reevaluate S_DAX when we change the RT
flag.

> Which brings me to the real problem here: dynamically changing the
> S_DAX flag is racy, dangerous and broken. It's not clear that this
> should be allowed at all as the inode may have already been mmap()d
> by the time the ioctl is called to set/clear the rt file state.

Agreed that this is a mess.  Either this needs to get fixed in the dax
code, or we need to decide that we're not going to support reconfiguring
the dax flag at all, except possibly for empty files (similar to how we
restrict changes to the rt flag).

> IOWs, right now we cannot support mixed DAX mode filesystems because
> the generic DAX code does not support dynamic changing of the DAX
> flag on an inode and so checking the block device state here is
> irrelevant....
> 
> >  		inode->i_flags |= S_DAX;
> >  }
> >  
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index e8a687232614..5ac478924dce 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1649,11 +1649,18 @@ xfs_fs_fill_super(
> >  		sb->s_flags |= SB_I_VERSION;
> >  
> >  	if (mp->m_flags & XFS_MOUNT_DAX) {
> > +		bool rtdev_is_dax = false;
> > +
> >  		xfs_warn(mp,
> >  		"DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
> >  
> > +		if (mp->m_rtdev_targp->bt_daxdev)
> > +			if (bdev_dax_supported(mp->m_rtdev_targp->bt_bdev,
> > +					      sb->s_blocksize) == 0)
> > +				rtdev_is_dax = true;
> 
> .... as this code here needs to turn off DAX here if any device
> in the filesystem doesn't support DAX....

I think it'd be useful to be able to have a pmem rt device even if the
data device doesn't support it.  Or rather, I have a few clients who
have expressed interest in this sort of configuration.

--D

> 
> 
> FWIW, the logic in the code is terrible (not your fault, Dave).
> The logic reads
> 
> 	if (NOT bdev_dax_supported(rtdev)) then
> 		rtdev supports DAX
> 
> That also needs fixing - we're checking something that has a boolean
> return state (yes or no) and so it should define them in a way that
> makes the caller logic read cleanly....
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@...morbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html