Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1739340 > unrolled thread

[PATCH 3/7] xfs: protect S_DAX transitions in XFS read path

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/7] xfs: protect S_DAX transitions in XFS read path Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-09-26 01:20 +0200
    Re: [PATCH 3/7] xfs: protect S_DAX transitions in XFS read path Dave Chinner <david@fromorbit.com> - 2017-09-26 02:10 +0200
    Re: [PATCH 3/7] xfs: protect S_DAX transitions in XFS read path Christoph Hellwig <hch@lst.de> - 2017-09-26 08:40 +0200
      Re: [PATCH 3/7] xfs: protect S_DAX transitions in XFS read path Dan Williams <dan.j.williams@intel.com> - 2017-09-26 16:10 +0200
        Re: [PATCH 3/7] xfs: protect S_DAX transitions in XFS read path Christoph Hellwig <hch@lst.de> - 2017-09-26 16:40 +0200
          Re: [PATCH 3/7] xfs: protect S_DAX transitions in XFS read path Dan Williams <dan.j.williams@intel.com> - 2017-09-26 20:20 +0200
            Re: [PATCH 3/7] xfs: protect S_DAX transitions in XFS read path Christoph Hellwig <hch@lst.de> - 2017-10-01 10:20 +0200

#1739340 — [PATCH 3/7] xfs: protect S_DAX transitions in XFS read path

In the current XFS read I/O path we check IS_DAX() in xfs_file_read_iter()
to decide whether to do DAX I/O, direct I/O or buffered I/O.  This check is
done without holding the XFS_IOLOCK, though, which means that if we allow
S_DAX to be manipulated via the inode flag we can run into this race:

CPU 0				CPU 1
-----				-----
xfs_file_read_iter()
  IS_DAX() << returns false
  				xfs_ioctl_setattr()
				  xfs_ioctl_setattr_dax_invalidate()
				   xfs_ilock(XFS_MMAPLOCK|XFS_IOLOCK)
				  sets S_DAX
				  releases XFS_MMAPLOCK and XFS_IOLOCK
  xfs_file_buffered_aio_read()
  does buffered I/O to DAX inode, death

Fix this by ensuring that we only check S_DAX when we hold the XFS_IOLOCK
in the read path.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 fs/xfs/xfs_file.c | 42 +++++++++++++-----------------------------
 1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index ebdd0bd..ca4c8fd 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -207,7 +207,6 @@ xfs_file_dio_aio_read(
 {
 	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
 	size_t			count = iov_iter_count(to);
-	ssize_t			ret;
 
 	trace_xfs_file_direct_read(ip, count, iocb->ki_pos);
 
@@ -215,12 +214,7 @@ xfs_file_dio_aio_read(
 		return 0; /* skip atime */
 
 	file_accessed(iocb->ki_filp);
-
-	xfs_ilock(ip, XFS_IOLOCK_SHARED);
-	ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL);
-	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
-
-	return ret;
+	return iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL);
 }
 
 static noinline ssize_t
@@ -230,23 +224,14 @@ xfs_file_dax_read(
 {
 	struct xfs_inode	*ip = XFS_I(iocb->ki_filp->f_mapping->host);
 	size_t			count = iov_iter_count(to);
-	ssize_t			ret = 0;
 
 	trace_xfs_file_dax_read(ip, count, iocb->ki_pos);
 
 	if (!count)
 		return 0; /* skip atime */
 
-	if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED)) {
-		if (iocb->ki_flags & IOCB_NOWAIT)
-			return -EAGAIN;
-		xfs_ilock(ip, XFS_IOLOCK_SHARED);
-	}
-	ret = dax_iomap_rw(iocb, to, &xfs_iomap_ops);
-	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
-
 	file_accessed(iocb->ki_filp);
-	return ret;
+	return dax_iomap_rw(iocb, to, &xfs_iomap_ops);
 }
 
 STATIC ssize_t
@@ -255,19 +240,9 @@ xfs_file_buffered_aio_read(
 	struct iov_iter		*to)
 {
 	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
-	ssize_t			ret;
 
 	trace_xfs_file_buffered_read(ip, iov_iter_count(to), iocb->ki_pos);
-
-	if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED)) {
-		if (iocb->ki_flags & IOCB_NOWAIT)
-			return -EAGAIN;
-		xfs_ilock(ip, XFS_IOLOCK_SHARED);
-	}
-	ret = generic_file_read_iter(iocb, to);
-	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
-
-	return ret;
+	return generic_file_read_iter(iocb, to);
 }
 
 STATIC ssize_t
@@ -276,7 +251,8 @@ xfs_file_read_iter(
 	struct iov_iter		*to)
 {
 	struct inode		*inode = file_inode(iocb->ki_filp);
-	struct xfs_mount	*mp = XFS_I(inode)->i_mount;
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
 	ssize_t			ret = 0;
 
 	XFS_STATS_INC(mp, xs_read_calls);
@@ -284,6 +260,12 @@ xfs_file_read_iter(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
+	if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED)) {
+		if (iocb->ki_flags & IOCB_NOWAIT)
+			return -EAGAIN;
+		xfs_ilock(ip, XFS_IOLOCK_SHARED);
+	}
+
 	if (IS_DAX(inode))
 		ret = xfs_file_dax_read(iocb, to);
 	else if (iocb->ki_flags & IOCB_DIRECT)
@@ -291,6 +273,8 @@ xfs_file_read_iter(
 	else
 		ret = xfs_file_buffered_aio_read(iocb, to);
 
+	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
+
 	if (ret > 0)
 		XFS_STATS_ADD(mp, xs_read_bytes, ret);
 	return ret;
-- 
2.9.5

[toc] | [next] | [standalone]

#1739384

On Mon, Sep 25, 2017 at 05:14:00PM -0600, Ross Zwisler wrote:
> In the current XFS read I/O path we check IS_DAX() in xfs_file_read_iter()
> to decide whether to do DAX I/O, direct I/O or buffered I/O.  This check is
> done without holding the XFS_IOLOCK, though, which means that if we allow
> S_DAX to be manipulated via the inode flag we can run into this race:
> 
> CPU 0				CPU 1
> -----				-----
> xfs_file_read_iter()
>   IS_DAX() << returns false
>   				xfs_ioctl_setattr()
> 				  xfs_ioctl_setattr_dax_invalidate()
> 				   xfs_ilock(XFS_MMAPLOCK|XFS_IOLOCK)
> 				  sets S_DAX
> 				  releases XFS_MMAPLOCK and XFS_IOLOCK
>   xfs_file_buffered_aio_read()
>   does buffered I/O to DAX inode, death
> 
> Fix this by ensuring that we only check S_DAX when we hold the XFS_IOLOCK
> in the read path.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  fs/xfs/xfs_file.c | 42 +++++++++++++-----------------------------
>  1 file changed, 13 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index ebdd0bd..ca4c8fd 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -207,7 +207,6 @@ xfs_file_dio_aio_read(
>  {
>  	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
>  	size_t			count = iov_iter_count(to);
> -	ssize_t			ret;
>  
>  	trace_xfs_file_direct_read(ip, count, iocb->ki_pos);
>  
> @@ -215,12 +214,7 @@ xfs_file_dio_aio_read(
>  		return 0; /* skip atime */
>  
>  	file_accessed(iocb->ki_filp);
> -
> -	xfs_ilock(ip, XFS_IOLOCK_SHARED);
> -	ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL);
> -	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> -
> -	return ret;
> +	return iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL);

This puts file_accessed under the XFS_IOLOCK_SHARED now. Is that a
safe/sane thing to do for DIO?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1739546

We can't just take locking one level up, as we need differnet locking
for different kinds of I/O.

I think you probably want an IOCB_DAX flag to check IS_DAX once and
then stick to it, similar to what we do for direct I/O.

[toc] | [prev] | [next] | [standalone]

#1739916

On Mon, Sep 25, 2017 at 11:32 PM, Christoph Hellwig <hch@lst.de> wrote:
> We can't just take locking one level up, as we need differnet locking
> for different kinds of I/O.
>
> I think you probably want an IOCB_DAX flag to check IS_DAX once and
> then stick to it, similar to what we do for direct I/O.

I wonder if this works better with a reference count mechanism
per-file so that we don't need a hold a lock over the whole
transition. Similar to request_queue reference counting, when DAX is
being turned off we block new references and drain the in-flight ones.

[toc] | [prev] | [next] | [standalone]

#1739945

On Tue, Sep 26, 2017 at 06:59:37AM -0700, Dan Williams wrote:
> > I think you probably want an IOCB_DAX flag to check IS_DAX once and
> > then stick to it, similar to what we do for direct I/O.
> 
> I wonder if this works better with a reference count mechanism
> per-file so that we don't need a hold a lock over the whole
> transition. Similar to request_queue reference counting, when DAX is
> being turned off we block new references and drain the in-flight ones.

Maybe.  But that assumes we want to be stuck in a perpetual binary
DAX on/off state on a given file.  Which makes not only for an awkward
interface (inode or mount flag), but also might be fundamentally the
wrong thing to do for some media where you'd happily read directly
from it but rather buffer writes in DRAM.

[toc] | [prev] | [next] | [standalone]

#1740098

On Tue, Sep 26, 2017 at 7:33 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Tue, Sep 26, 2017 at 06:59:37AM -0700, Dan Williams wrote:
>> > I think you probably want an IOCB_DAX flag to check IS_DAX once and
>> > then stick to it, similar to what we do for direct I/O.
>>
>> I wonder if this works better with a reference count mechanism
>> per-file so that we don't need a hold a lock over the whole
>> transition. Similar to request_queue reference counting, when DAX is
>> being turned off we block new references and drain the in-flight ones.
>
> Maybe.  But that assumes we want to be stuck in a perpetual binary
> DAX on/off state on a given file.  Which makes not only for an awkward
> interface (inode or mount flag), but also might be fundamentally the
> wrong thing to do for some media where you'd happily read directly
> from it but rather buffer writes in DRAM.

I think we'll always need an explicit override available, but yes we
need to think about what the override looks like in the context of a
kernel that is able to automatically pick the right I/O policy
relative to the media type. A potential mixed policy for reads vs
writes makes sense. Where would this finer grained I/O policy
selection go other than more inode flags?

[toc] | [prev] | [next] | [standalone]

#1742749

On Tue, Sep 26, 2017 at 11:11:55AM -0700, Dan Williams wrote:
> I think we'll always need an explicit override available, but yes we
> need to think about what the override looks like in the context of a
> kernel that is able to automatically pick the right I/O policy
> relative to the media type. A potential mixed policy for reads vs
> writes makes sense. Where would this finer grained I/O policy
> selection go other than more inode flags?

fadvise?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web