| Message ID | 20211018044054.1779424-7-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] dm: make the DAX support dependend on CONFIG_FS_DAX | expand |
On Mon, Oct 18, 2021 at 06:40:49AM +0200, Christoph Hellwig wrote: > Factor out another DAX setup helper to simplify future changes. Also > move the experimental warning after the checks to not clutter the log > too much if the setup failed. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_super.c | 47 +++++++++++++++++++++++++++------------------- > 1 file changed, 28 insertions(+), 19 deletions(-) > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index c4e0cd1c1c8ca..d07020a8eb9e3 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -339,6 +339,32 @@ xfs_buftarg_is_dax( > bdev_nr_sectors(bt->bt_bdev)); > } > > +static int > +xfs_setup_dax( /me wonders if this should be named xfs_setup_dax_always, since this doesn't handle the dax=inode mode? The only reason I bring that up is that Eric reminded me a while ago that we don't actually print any kind of EXPERIMENTAL warning for the auto-detection behavior. That said, I never liked looking at the nested backwards logic of the old code, so: Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > + struct xfs_mount *mp) > +{ > + struct super_block *sb = mp->m_super; > + > + if (!xfs_buftarg_is_dax(sb, mp->m_ddev_targp) && > + (!mp->m_rtdev_targp || !xfs_buftarg_is_dax(sb, mp->m_rtdev_targp))) { > + xfs_alert(mp, > + "DAX unsupported by block device. Turning off DAX."); > + goto disable_dax; > + } > + > + if (xfs_has_reflink(mp)) { > + xfs_alert(mp, "DAX and reflink cannot be used together!"); > + return -EINVAL; > + } > + > + xfs_warn(mp, "DAX enabled. Warning: EXPERIMENTAL, use at your own risk"); > + return 0; > + > +disable_dax: > + xfs_mount_set_dax_mode(mp, XFS_DAX_NEVER); > + return 0; > +} > + > STATIC int > xfs_blkdev_get( > xfs_mount_t *mp, > @@ -1592,26 +1618,9 @@ xfs_fs_fill_super( > sb->s_flags |= SB_I_VERSION; > > if (xfs_has_dax_always(mp)) { > - bool rtdev_is_dax = false, datadev_is_dax; > - > - xfs_warn(mp, > - "DAX enabled. Warning: EXPERIMENTAL, use at your own risk"); > - > - datadev_is_dax = xfs_buftarg_is_dax(sb, mp->m_ddev_targp); > - if (mp->m_rtdev_targp) > - rtdev_is_dax = xfs_buftarg_is_dax(sb, > - mp->m_rtdev_targp); > - if (!rtdev_is_dax && !datadev_is_dax) { > - xfs_alert(mp, > - "DAX unsupported by block device. Turning off DAX."); > - xfs_mount_set_dax_mode(mp, XFS_DAX_NEVER); > - } > - if (xfs_has_reflink(mp)) { > - xfs_alert(mp, > - "DAX and reflink cannot be used together!"); > - error = -EINVAL; > + error = xfs_setup_dax(mp); > + if (error) > goto out_filestream_unmount; > - } > } > > if (xfs_has_discard(mp)) { > -- > 2.30.2 >
On Mon, Oct 18, 2021 at 09:43:51AM -0700, Darrick J. Wong wrote: > > --- a/fs/xfs/xfs_super.c > > +++ b/fs/xfs/xfs_super.c > > @@ -339,6 +339,32 @@ xfs_buftarg_is_dax( > > bdev_nr_sectors(bt->bt_bdev)); > > } > > > > +static int > > +xfs_setup_dax( > > /me wonders if this should be named xfs_setup_dax_always, since this > doesn't handle the dax=inode mode? Sure, why not. > The only reason I bring that up is that Eric reminded me a while ago > that we don't actually print any kind of EXPERIMENTAL warning for the > auto-detection behavior. Yes, I actually noticed that as well when preparing this series.
On Tue, Oct 19, 2021 at 12:24 AM Christoph Hellwig <hch@lst.de> wrote: > > On Mon, Oct 18, 2021 at 09:43:51AM -0700, Darrick J. Wong wrote: > > > --- a/fs/xfs/xfs_super.c > > > +++ b/fs/xfs/xfs_super.c > > > @@ -339,6 +339,32 @@ xfs_buftarg_is_dax( > > > bdev_nr_sectors(bt->bt_bdev)); > > > } > > > > > > +static int > > > +xfs_setup_dax( > > > > /me wonders if this should be named xfs_setup_dax_always, since this > > doesn't handle the dax=inode mode? > > Sure, why not. I went ahead and made that change locally. > > > The only reason I bring that up is that Eric reminded me a while ago > > that we don't actually print any kind of EXPERIMENTAL warning for the > > auto-detection behavior. > > Yes, I actually noticed that as well when preparing this series. The rest looks good to me.
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index c4e0cd1c1c8ca..d07020a8eb9e3 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -339,6 +339,32 @@ xfs_buftarg_is_dax( bdev_nr_sectors(bt->bt_bdev)); } +static int +xfs_setup_dax( + struct xfs_mount *mp) +{ + struct super_block *sb = mp->m_super; + + if (!xfs_buftarg_is_dax(sb, mp->m_ddev_targp) && + (!mp->m_rtdev_targp || !xfs_buftarg_is_dax(sb, mp->m_rtdev_targp))) { + xfs_alert(mp, + "DAX unsupported by block device. Turning off DAX."); + goto disable_dax; + } + + if (xfs_has_reflink(mp)) { + xfs_alert(mp, "DAX and reflink cannot be used together!"); + return -EINVAL; + } + + xfs_warn(mp, "DAX enabled. Warning: EXPERIMENTAL, use at your own risk"); + return 0; + +disable_dax: + xfs_mount_set_dax_mode(mp, XFS_DAX_NEVER); + return 0; +} + STATIC int xfs_blkdev_get( xfs_mount_t *mp, @@ -1592,26 +1618,9 @@ xfs_fs_fill_super( sb->s_flags |= SB_I_VERSION; if (xfs_has_dax_always(mp)) { - bool rtdev_is_dax = false, datadev_is_dax; - - xfs_warn(mp, - "DAX enabled. Warning: EXPERIMENTAL, use at your own risk"); - - datadev_is_dax = xfs_buftarg_is_dax(sb, mp->m_ddev_targp); - if (mp->m_rtdev_targp) - rtdev_is_dax = xfs_buftarg_is_dax(sb, - mp->m_rtdev_targp); - if (!rtdev_is_dax && !datadev_is_dax) { - xfs_alert(mp, - "DAX unsupported by block device. Turning off DAX."); - xfs_mount_set_dax_mode(mp, XFS_DAX_NEVER); - } - if (xfs_has_reflink(mp)) { - xfs_alert(mp, - "DAX and reflink cannot be used together!"); - error = -EINVAL; + error = xfs_setup_dax(mp); + if (error) goto out_filestream_unmount; - } } if (xfs_has_discard(mp)) {
Factor out another DAX setup helper to simplify future changes. Also move the experimental warning after the checks to not clutter the log too much if the setup failed. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_super.c | 47 +++++++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 19 deletions(-)