diff mbox series

xfs: conditionally allow FS_XFLAG_REALTIME changes if S_DAX is set

Message ID 20240805184645.GC623936@frogsfrogsfrogs (mailing list archive)
State Accepted, archived
Headers show
Series xfs: conditionally allow FS_XFLAG_REALTIME changes if S_DAX is set | expand

Commit Message

Darrick J. Wong Aug. 5, 2024, 6:46 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

If a file has the S_DAX flag (aka fsdax access mode) set, we cannot
allow users to change the realtime flag unless the datadev and rtdev
both support fsdax access modes.  Even if there are no extents allocated
to the file, the setattr thread could be racing with another thread
that has already started down the write code paths.

Fixes: ba23cba9b3bdc ("fs: allow per-device dax status checking for filesystems")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_ioctl.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Christoph Hellwig Aug. 6, 2024, 1:07 p.m. UTC | #1 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Souptick Joarder Aug. 10, 2024, 2:48 p.m. UTC | #2 
On Tue, Aug 6, 2024 at 12:16 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> From: Darrick J. Wong <djwong@kernel.org>
>
> If a file has the S_DAX flag (aka fsdax access mode) set, we cannot
> allow users to change the realtime flag unless the datadev and rtdev
> both support fsdax access modes.  Even if there are no extents allocated
> to the file, the setattr thread could be racing with another thread
> that has already started down the write code paths.
>
> Fixes: ba23cba9b3bdc ("fs: allow per-device dax status checking for filesystems")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_ioctl.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 4e933db75b12..6b13666d4e96 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -483,6 +483,17 @@ xfs_ioctl_setattr_xflags(
>                 /* Can't change realtime flag if any extents are allocated. */
>                 if (ip->i_df.if_nextents || ip->i_delayed_blks)
>                         return -EINVAL;
> +
> +               /*
> +                * If S_DAX is enabled on this file, we can only switch the
> +                * device if both support fsdax.  We can't update S_DAX because
> +                * there might be other threads walking down the access paths.
> +                */
> +               if (IS_DAX(VFS_I(ip)) &&
> +                   (mp->m_ddev_targp->bt_daxdev == NULL ||
> +                    (mp->m_rtdev_targp &&
> +                     mp->m_rtdev_targp->bt_daxdev == NULL)))
> +                       return -EINVAL;
Any chance of  mp->m_ddev_targp == NULL  ?

>         }
>
>         if (rtflag) {
>
Darrick J. Wong Aug. 10, 2024, 4:41 p.m. UTC | #3 
On Sat, Aug 10, 2024 at 08:18:16PM +0530, Souptick Joarder wrote:
> On Tue, Aug 6, 2024 at 12:16 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > If a file has the S_DAX flag (aka fsdax access mode) set, we cannot
> > allow users to change the realtime flag unless the datadev and rtdev
> > both support fsdax access modes.  Even if there are no extents allocated
> > to the file, the setattr thread could be racing with another thread
> > that has already started down the write code paths.
> >
> > Fixes: ba23cba9b3bdc ("fs: allow per-device dax status checking for filesystems")
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_ioctl.c |   11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 4e933db75b12..6b13666d4e96 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -483,6 +483,17 @@ xfs_ioctl_setattr_xflags(
> >                 /* Can't change realtime flag if any extents are allocated. */
> >                 if (ip->i_df.if_nextents || ip->i_delayed_blks)
> >                         return -EINVAL;
> > +
> > +               /*
> > +                * If S_DAX is enabled on this file, we can only switch the
> > +                * device if both support fsdax.  We can't update S_DAX because
> > +                * there might be other threads walking down the access paths.
> > +                */
> > +               if (IS_DAX(VFS_I(ip)) &&
> > +                   (mp->m_ddev_targp->bt_daxdev == NULL ||
> > +                    (mp->m_rtdev_targp &&
> > +                     mp->m_rtdev_targp->bt_daxdev == NULL)))
> > +                       return -EINVAL;
> Any chance of  mp->m_ddev_targp == NULL  ?

No, m_ddev_targp is the device you give to mount. e.g. if you do

# mount /dev/sda /mnt

then m_ddev_targp is the block_device /dev/sda.

--D

> >         }
> >
> >         if (rtflag) {
> >
diff mbox series

Patch

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 4e933db75b12..6b13666d4e96 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -483,6 +483,17 @@  xfs_ioctl_setattr_xflags(
 		/* Can't change realtime flag if any extents are allocated. */
 		if (ip->i_df.if_nextents || ip->i_delayed_blks)
 			return -EINVAL;
+
+		/*
+		 * If S_DAX is enabled on this file, we can only switch the
+		 * device if both support fsdax.  We can't update S_DAX because
+		 * there might be other threads walking down the access paths.
+		 */
+		if (IS_DAX(VFS_I(ip)) &&
+		    (mp->m_ddev_targp->bt_daxdev == NULL ||
+		     (mp->m_rtdev_targp &&
+		      mp->m_rtdev_targp->bt_daxdev == NULL)))
+			return -EINVAL;
 	}
 
 	if (rtflag) {