[PATCH 0/5] extent list locking fixes

[PATCH 0/5] extent list locking fixes

[Christoph Hellwig]

As discussed in the getdents locking thread I think we should assert
the we have the right locking for reading in the extent list and
the xfs_bmap_* operations.

This series contains a patch to add those asserts (the last one),
as well a the fallout it detected.  This is in addition to the getdents
patch from Ben.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/5] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes

[Christoph Hellwig]

[-- Attachment #1: xfs_zero_remaining_bytes-locking --]
[-- Type: text/plain, Size: 1183 bytes --]

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_bmap_util.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.c	2013-12-05 11:37:57.791685284 +0100
+++ xfs/fs/xfs/xfs_bmap_util.c	2013-12-05 11:39:43.599683113 +0100
@@ -1147,6 +1147,7 @@ xfs_zero_remaining_bytes(
 	xfs_mount_t		*mp = ip->i_mount;
 	int			nimap;
 	int			error = 0;
+	uint			lock_mode;
 
 	/*
 	 * Avoid doing I/O beyond eof - it's not necessary
@@ -1159,11 +1160,15 @@ xfs_zero_remaining_bytes(
 	if (endoff > XFS_ISIZE(ip))
 		endoff = XFS_ISIZE(ip);
 
+	lock_mode = xfs_ilock_map_shared(ip);
+
 	bp = xfs_buf_get_uncached(XFS_IS_REALTIME_INODE(ip) ?
 					mp->m_rtdev_targp : mp->m_ddev_targp,
 				  BTOBB(mp->m_sb.sb_blocksize), 0);
-	if (!bp)
-		return XFS_ERROR(ENOMEM);
+	if (!bp) {
+		error = XFS_ERROR(ENOMEM);
+		goto out_unlock;
+	}
 
 	xfs_buf_unlock(bp);
 
@@ -1209,6 +1214,8 @@ xfs_zero_remaining_bytes(
 		}
 	}
 	xfs_buf_free(bp);
+out_unlock:
+	xfs_iunlock_map_shared(ip, lock_mode);
 	return error;
 }
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/5] xfs: use xfs_ilock_map_shared in xfs_qm_dqtobp

[Christoph Hellwig]

[-- Attachment #1: xfs-quota-locking --]
[-- Type: text/plain, Size: 1417 bytes --]

We might not have read in the extent list at this point, so make sure we
take the ilock exclusively if we have to do so.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.c	2013-11-18 14:39:01.955589999 +0100
+++ xfs/fs/xfs/xfs_dquot.c	2013-12-05 11:42:34.759679600 +0100
@@ -469,16 +469,17 @@ xfs_qm_dqtobp(
 	struct xfs_mount	*mp = dqp->q_mount;
 	xfs_dqid_t		id = be32_to_cpu(dqp->q_core.d_id);
 	struct xfs_trans	*tp = (tpp ? *tpp : NULL);
+	uint			lock_mode;
 
 	dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk;
 
-	xfs_ilock(quotip, XFS_ILOCK_SHARED);
+	lock_mode = xfs_ilock_map_shared(quotip);
 	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
 		/*
 		 * Return if this type of quotas is turned off while we
 		 * didn't have the quota inode lock.
 		 */
-		xfs_iunlock(quotip, XFS_ILOCK_SHARED);
+		xfs_iunlock_map_shared(quotip, lock_mode);
 		return ESRCH;
 	}
 
@@ -488,7 +489,7 @@ xfs_qm_dqtobp(
 	error = xfs_bmapi_read(quotip, dqp->q_fileoffset,
 			       XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0);
 
-	xfs_iunlock(quotip, XFS_ILOCK_SHARED);
+	xfs_iunlock_map_shared(quotip, lock_mode);
 	if (error)
 		return error;
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/5] xfs: use xfs_ilock_map_shared in xfs_qm_dqiterate

[Christoph Hellwig]

[-- Attachment #1: xfs-quota-locking-2 --]
[-- Type: text/plain, Size: 1142 bytes --]

We might not have read in the extent list at this point, so make sure we
take the ilock exclusively if we have to do so.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c	2013-11-18 14:39:01.967589998 +0100
+++ xfs/fs/xfs/xfs_qm.c	2013-12-05 12:32:36.623617997 +0100
@@ -1193,16 +1193,18 @@ xfs_qm_dqiterate(
 	lblkno = 0;
 	maxlblkcnt = XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes);
 	do {
+		uint		lock_mode;
+
 		nmaps = XFS_DQITER_MAP_SIZE;
 		/*
 		 * We aren't changing the inode itself. Just changing
 		 * some of its data. No new blocks are added here, and
 		 * the inode is never added to the transaction.
 		 */
-		xfs_ilock(qip, XFS_ILOCK_SHARED);
+		lock_mode = xfs_ilock_map_shared(qip);
 		error = xfs_bmapi_read(qip, lblkno, maxlblkcnt - lblkno,
 				       map, &nmaps, 0);
-		xfs_iunlock(qip, XFS_ILOCK_SHARED);
+		xfs_iunlock_map_shared(qip, lock_mode);
 		if (error)
 			break;
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/5] xfs: use xfs_ilock_map_shared in xfs_attr_get

[Christoph Hellwig]

[-- Attachment #1: xfs-fix-attr-locking --]
[-- Type: text/plain, Size: 855 bytes --]

We might not have read in the extent list at this point, so make sure we
take the ilock exclusively if we have to do so.

Signed-off-by: Christoph Hellwig <hch@lst.de>

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index b861270..5343034 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -164,6 +164,7 @@ xfs_attr_get(
 {
 	int		error;
 	struct xfs_name	xname;
+	uint		lock_mode;
 
 	XFS_STATS_INC(xs_attr_get);
 
@@ -174,9 +175,9 @@ xfs_attr_get(
 	if (error)
 		return error;
 
-	xfs_ilock(ip, XFS_ILOCK_SHARED);
+	lock_mode = xfs_ilock_map_shared(ip);
 	error = xfs_attr_get_int(ip, &xname, value, valuelenp, flags);
-	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+	xfs_iunlock_map_shared(ip, lock_mode);
 	return(error);
 }
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 5/5] xfs: assert that we hold the ilock for extent map access

[Christoph Hellwig]

[-- Attachment #1: iread_extents-assert --]
[-- Type: text/plain, Size: 1989 bytes --]

Make sure that xfs_bmapi_read has the ilock held in some way, and that
xfs_bmapi_write, xfs_bmapi_delay and xfs_iread_extents are called with
the ilock held exclusively.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_bmap.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap.c	2013-11-29 14:25:12.172459195 +0100
+++ xfs/fs/xfs/xfs_bmap.c	2013-12-05 10:03:28.243801633 +0100
@@ -3997,6 +3997,7 @@ xfs_bmapi_read(
 	ASSERT(*nmap >= 1);
 	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK|XFS_BMAPI_ENTIRE|
 			   XFS_BMAPI_IGSTATE)));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
 
 	if (unlikely(XFS_TEST_ERROR(
 	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
@@ -4191,6 +4192,7 @@ xfs_bmapi_delay(
 	ASSERT(*nmap >= 1);
 	ASSERT(*nmap <= XFS_BMAP_MAX_NMAP);
 	ASSERT(!(flags & ~XFS_BMAPI_ENTIRE));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
 	if (unlikely(XFS_TEST_ERROR(
 	    (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
@@ -4484,6 +4486,7 @@ xfs_bmapi_write(
 	ASSERT(tp != NULL);
 	ASSERT(len > 0);
 	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
 	if (unlikely(XFS_TEST_ERROR(
 	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
Index: xfs/fs/xfs/xfs_inode_fork.c
===================================================================
--- xfs.orig/fs/xfs/xfs_inode_fork.c	2013-12-05 09:57:24.347809100 +0100
+++ xfs/fs/xfs/xfs_inode_fork.c	2013-12-05 09:59:04.767807039 +0100
@@ -431,6 +431,8 @@ xfs_iread_extents(
 	xfs_ifork_t	*ifp;
 	xfs_extnum_t	nextents;
 
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+
 	if (unlikely(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
 		XFS_ERROR_REPORT("xfs_iread_extents", XFS_ERRLEVEL_LOW,
 				 ip->i_mount);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/5] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes

[Ben Myers]

On Thu, Dec 05, 2013 at 07:58:31AM -0800, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

For fallocate and ioc space.  Looks good.

Reviewed-by: Ben Myers <bpm@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/5] xfs: use xfs_ilock_map_shared in xfs_qm_dqtobp

[Ben Myers]

On Thu, Dec 05, 2013 at 07:58:32AM -0800, Christoph Hellwig wrote:
> We might not have read in the extent list at this point, so make sure we
> take the ilock exclusively if we have to do so.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ben Myers <bpm@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/5] xfs: use xfs_ilock_map_shared in xfs_qm_dqiterate

[Ben Myers]

On Thu, Dec 05, 2013 at 07:58:33AM -0800, Christoph Hellwig wrote:
> We might not have read in the extent list at this point, so make sure we
> take the ilock exclusively if we have to do so.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ben Myers <bpm@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/5] xfs: use xfs_ilock_map_shared in xfs_attr_get

[Ben Myers]

On Thu, Dec 05, 2013 at 07:58:34AM -0800, Christoph Hellwig wrote:
> We might not have read in the extent list at this point, so make sure we
> take the ilock exclusively if we have to do so.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ben Myers <bpm@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/5] xfs: assert that we hold the ilock for extent map access

[Ben Myers]

On Thu, Dec 05, 2013 at 07:58:35AM -0800, Christoph Hellwig wrote:
> Make sure that xfs_bmapi_read has the ilock held in some way, and that
> xfs_bmapi_write, xfs_bmapi_delay and xfs_iread_extents are called with
> the ilock held exclusively.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ben Myers <bpm@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/5] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes

[Dave Chinner]

On Thu, Dec 05, 2013 at 07:58:31AM -0800, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_bmap_util.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap_util.c	2013-12-05 11:37:57.791685284 +0100
> +++ xfs/fs/xfs/xfs_bmap_util.c	2013-12-05 11:39:43.599683113 +0100
> @@ -1147,6 +1147,7 @@ xfs_zero_remaining_bytes(
>  	xfs_mount_t		*mp = ip->i_mount;
>  	int			nimap;
>  	int			error = 0;
> +	uint			lock_mode;
>  
>  	/*
>  	 * Avoid doing I/O beyond eof - it's not necessary
> @@ -1159,11 +1160,15 @@ xfs_zero_remaining_bytes(
>  	if (endoff > XFS_ISIZE(ip))
>  		endoff = XFS_ISIZE(ip);
>  
> +	lock_mode = xfs_ilock_map_shared(ip);
> +
>  	bp = xfs_buf_get_uncached(XFS_IS_REALTIME_INODE(ip) ?
>  					mp->m_rtdev_targp : mp->m_ddev_targp,
>  				  BTOBB(mp->m_sb.sb_blocksize), 0);

This now holds the ilock over data IO, which is not allowed to be
done as data IO completion can require the ilock to be taken. Yes,
the code specifically avoids all these problems, but the general
rule is that ilock is only held over metadata IO operations, not
data IO. If we need data IO serialisation, then we use the iolock.

So, while this protects the extent tree, it also violates other
long-standing conventions for locking. Given that the code is
special, I'mnot opposed to making a special rule for this one
function, but it needs to be commented as to why this is a valid
thing to do in this function....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/5] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes

[Ben Myers]

On Fri, Dec 06, 2013 at 07:31:15AM +1100, Dave Chinner wrote:
> On Thu, Dec 05, 2013 at 07:58:31AM -0800, Christoph Hellwig wrote:
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> > Index: xfs/fs/xfs/xfs_bmap_util.c
> > ===================================================================
> > --- xfs.orig/fs/xfs/xfs_bmap_util.c	2013-12-05 11:37:57.791685284 +0100
> > +++ xfs/fs/xfs/xfs_bmap_util.c	2013-12-05 11:39:43.599683113 +0100
> > @@ -1147,6 +1147,7 @@ xfs_zero_remaining_bytes(
> >  	xfs_mount_t		*mp = ip->i_mount;
> >  	int			nimap;
> >  	int			error = 0;
> > +	uint			lock_mode;
> >  
> >  	/*
> >  	 * Avoid doing I/O beyond eof - it's not necessary
> > @@ -1159,11 +1160,15 @@ xfs_zero_remaining_bytes(
> >  	if (endoff > XFS_ISIZE(ip))
> >  		endoff = XFS_ISIZE(ip);
> >  
> > +	lock_mode = xfs_ilock_map_shared(ip);
> > +
> >  	bp = xfs_buf_get_uncached(XFS_IS_REALTIME_INODE(ip) ?
> >  					mp->m_rtdev_targp : mp->m_ddev_targp,
> >  				  BTOBB(mp->m_sb.sb_blocksize), 0);
> 
> This now holds the ilock over data IO, which is not allowed to be
> done as data IO completion can require the ilock to be taken. Yes,
> the code specifically avoids all these problems, but the general
> rule is that ilock is only held over metadata IO operations, not
> data IO. If we need data IO serialisation, then we use the iolock.
> 
> So, while this protects the extent tree, it also violates other
> long-standing conventions for locking. Given that the code is
> special, I'mnot opposed to making a special rule for this one
> function, but it needs to be commented as to why this is a valid
> thing to do in this function....

Maybe it would be better if the ilock could be taken and dropped within the
loop.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/5] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes

[Christoph Hellwig]

On Fri, Dec 06, 2013 at 07:31:15AM +1100, Dave Chinner wrote:
> This now holds the ilock over data IO, which is not allowed to be
> done as data IO completion can require the ilock to be taken. Yes,
> the code specifically avoids all these problems, but the general
> rule is that ilock is only held over metadata IO operations, not
> data IO. If we need data IO serialisation, then we use the iolock.

And we already hold the iolock here.  So yeah, we probably should
just move it to protect the xfs_bmapi_read call only.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/5] xfs: use xfs_ilock_map_shared in xfs_qm_dqtobp

[Dave Chinner]

On Thu, Dec 05, 2013 at 07:58:32AM -0800, Christoph Hellwig wrote:
> We might not have read in the extent list at this point, so make sure we
> take the ilock exclusively if we have to do so.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_dquot.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_dquot.c	2013-11-18 14:39:01.955589999 +0100
> +++ xfs/fs/xfs/xfs_dquot.c	2013-12-05 11:42:34.759679600 +0100
> @@ -469,16 +469,17 @@ xfs_qm_dqtobp(
>  	struct xfs_mount	*mp = dqp->q_mount;
>  	xfs_dqid_t		id = be32_to_cpu(dqp->q_core.d_id);
>  	struct xfs_trans	*tp = (tpp ? *tpp : NULL);
> +	uint			lock_mode;
>  
>  	dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk;
>  
> -	xfs_ilock(quotip, XFS_ILOCK_SHARED);
> +	lock_mode = xfs_ilock_map_shared(quotip);
>  	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
>  		/*
>  		 * Return if this type of quotas is turned off while we
>  		 * didn't have the quota inode lock.
>  		 */
> -		xfs_iunlock(quotip, XFS_ILOCK_SHARED);
> +		xfs_iunlock_map_shared(quotip, lock_mode);
>  		return ESRCH;
>  	}
>  
> @@ -488,7 +489,7 @@ xfs_qm_dqtobp(
>  	error = xfs_bmapi_read(quotip, dqp->q_fileoffset,
>  			       XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0);
>  
> -	xfs_iunlock(quotip, XFS_ILOCK_SHARED);
> +	xfs_iunlock_map_shared(quotip, lock_mode);
>  	if (error)
>  		return error;

Looks ok, so consider it:

Reviewed-by: Dave Chinner <dchinner@redhat.com>

However, it raises a bigger question about dquot allocation sanity
to me: what makes the map returned valid after we've unlocked the
extent list?

We then use it to determine whether to allocate a
dquot or not, and xfs_qm_dqalloc() then does this after calling
xfs_bmapi_write():

	ASSERT((map.br_startblock != DELAYSTARTBLOCK) &&
	       (map.br_startblock != HOLESTARTBLOCK));

What's to prevent someone coming in between the xfs_bmapi_read()
and *write() calls and allocating a different dquot in the same
cluster and therefore beating the first thread to the allocation?

This read/write race exists elsewhere - e.g. xfs_iomap_write_allocate
documents it for the data path - and it has to be specifically
handled to prevent corruption.....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/5] xfs: use xfs_ilock_map_shared in xfs_qm_dqiterate

[Dave Chinner]

On Thu, Dec 05, 2013 at 07:58:33AM -0800, Christoph Hellwig wrote:
> We might not have read in the extent list at this point, so make sure we
> take the ilock exclusively if we have to do so.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_qm.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_qm.c	2013-11-18 14:39:01.967589998 +0100
> +++ xfs/fs/xfs/xfs_qm.c	2013-12-05 12:32:36.623617997 +0100
> @@ -1193,16 +1193,18 @@ xfs_qm_dqiterate(
>  	lblkno = 0;
>  	maxlblkcnt = XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes);
>  	do {
> +		uint		lock_mode;
> +
>  		nmaps = XFS_DQITER_MAP_SIZE;
>  		/*
>  		 * We aren't changing the inode itself. Just changing
>  		 * some of its data. No new blocks are added here, and
>  		 * the inode is never added to the transaction.
>  		 */
> -		xfs_ilock(qip, XFS_ILOCK_SHARED);
> +		lock_mode = xfs_ilock_map_shared(qip);
>  		error = xfs_bmapi_read(qip, lblkno, maxlblkcnt - lblkno,
>  				       map, &nmaps, 0);
> -		xfs_iunlock(qip, XFS_ILOCK_SHARED);
> +		xfs_iunlock_map_shared(qip, lock_mode);
>  		if (error)
>  			break;

There's no bug here - this comes from quotacheck, which is
guaranteed to have exclusive access to the filesystem at this point.
Hence there's no-one to race with reading the extent list. Still, it
doesn't hurt.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/5] xfs: use xfs_ilock_map_shared in xfs_qm_dqtobp

[Christoph Hellwig]

On Fri, Dec 06, 2013 at 07:41:08AM +1100, Dave Chinner wrote:
> However, it raises a bigger question about dquot allocation sanity
> to me: what makes the map returned valid after we've unlocked the
> extent list?
> 
> We then use it to determine whether to allocate a
> dquot or not, and xfs_qm_dqalloc() then does this after calling
> xfs_bmapi_write():
> 
> 	ASSERT((map.br_startblock != DELAYSTARTBLOCK) &&
> 	       (map.br_startblock != HOLESTARTBLOCK));
> 
> What's to prevent someone coming in between the xfs_bmapi_read()
> and *write() calls and allocating a different dquot in the same
> cluster and therefore beating the first thread to the allocation?
> 
> This read/write race exists elsewhere - e.g. xfs_iomap_write_allocate
> documents it for the data path - and it has to be specifically
> handled to prevent corruption.....

Yeah, we'll need to read-read the extent map in xfs_qm_dqalloc. I  I
think this is efficiently paper over by the buffer lock - we take
it right after the xfs_bmapi_write over the period of initialization
the on-disk dquots and copying the one we were called for into the
in-memory one.  So while we might have been corrupting dquots all
over no one has noticed because we had a non-corrupted version
in-memory that overwrote the corrupted one again later.  Uhh..

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/5] xfs: use xfs_ilock_map_shared in xfs_attr_get

[Dave Chinner]

On Thu, Dec 05, 2013 at 07:58:34AM -0800, Christoph Hellwig wrote:
> We might not have read in the extent list at this point, so make sure we
> take the ilock exclusively if we have to do so.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
> index b861270..5343034 100644
> --- a/fs/xfs/xfs_attr.c
> +++ b/fs/xfs/xfs_attr.c
> @@ -164,6 +164,7 @@ xfs_attr_get(
>  {
>  	int		error;
>  	struct xfs_name	xname;
> +	uint		lock_mode;
>  
>  	XFS_STATS_INC(xs_attr_get);
>  
> @@ -174,9 +175,9 @@ xfs_attr_get(
>  	if (error)
>  		return error;
>  
> -	xfs_ilock(ip, XFS_ILOCK_SHARED);
> +	lock_mode = xfs_ilock_map_shared(ip);
>  	error = xfs_attr_get_int(ip, &xname, value, valuelenp, flags);
> -	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +	xfs_iunlock_map_shared(ip, lock_mode);
>  	return(error);
>  }

I think the locking here should be moved inside xfs_attr_get_int()
so that it uses the same locking pattern as xfs_attr_set() and
xfs_attr_remove().

Also, xfs_attr_list() needs this treatment (the attr version of
readdir) as well (and it has the locking inside xfs_attr_list_int(),
too ;).

It looks like xfs_readlink needs fixing, too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/5] xfs: use xfs_ilock_map_shared in xfs_attr_get

[Christoph Hellwig]

On Fri, Dec 06, 2013 at 07:59:10AM +1100, Dave Chinner wrote:
> I think the locking here should be moved inside xfs_attr_get_int()

Or we could just kill xfs_attr_get_int..

> so that it uses the same locking pattern as xfs_attr_set() and
> xfs_attr_remove().
> 
> Also, xfs_attr_list() needs this treatment (the attr version of
> readdir) as well (and it has the locking inside xfs_attr_list_int(),
> too ;).
>
> It looks like xfs_readlink needs fixing, too.

Haven't really done an in-depth audit, mostly just looking at
where the asserts kick in..

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/5] xfs: use xfs_ilock_map_shared in xfs_qm_dqtobp

[Dave Chinner]

On Thu, Dec 05, 2013 at 12:53:45PM -0800, Christoph Hellwig wrote:
> On Fri, Dec 06, 2013 at 07:41:08AM +1100, Dave Chinner wrote:
> > However, it raises a bigger question about dquot allocation sanity
> > to me: what makes the map returned valid after we've unlocked the
> > extent list?
> > 
> > We then use it to determine whether to allocate a
> > dquot or not, and xfs_qm_dqalloc() then does this after calling
> > xfs_bmapi_write():
> > 
> > 	ASSERT((map.br_startblock != DELAYSTARTBLOCK) &&
> > 	       (map.br_startblock != HOLESTARTBLOCK));
> > 
> > What's to prevent someone coming in between the xfs_bmapi_read()
> > and *write() calls and allocating a different dquot in the same
> > cluster and therefore beating the first thread to the allocation?
> > 
> > This read/write race exists elsewhere - e.g. xfs_iomap_write_allocate
> > documents it for the data path - and it has to be specifically
> > handled to prevent corruption.....
> 
> Yeah, we'll need to read-read the extent map in xfs_qm_dqalloc. I  I
> think this is efficiently paper over by the buffer lock - we take
> it right after the xfs_bmapi_write over the period of initialization
> the on-disk dquots and copying the one we were called for into the
> in-memory one.  So while we might have been corrupting dquots all
> over no one has noticed because we had a non-corrupted version
> in-memory that overwrote the corrupted one again later.  Uhh..

Hmmm - I didn't think of the "rewrite dquot form memory into buffer"
aspect of it. That makes it even more subtle and less likely to be
seen unless we crash at exactly the wrong time...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/5] xfs: use xfs_ilock_map_shared in xfs_attr_get

[Dave Chinner]

On Thu, Dec 05, 2013 at 01:01:59PM -0800, Christoph Hellwig wrote:
> On Fri, Dec 06, 2013 at 07:59:10AM +1100, Dave Chinner wrote:
> > I think the locking here should be moved inside xfs_attr_get_int()
> 
> Or we could just kill xfs_attr_get_int..
> 
> > so that it uses the same locking pattern as xfs_attr_set() and
> > xfs_attr_remove().
> > 
> > Also, xfs_attr_list() needs this treatment (the attr version of
> > readdir) as well (and it has the locking inside xfs_attr_list_int(),
> > too ;).
> >
> > It looks like xfs_readlink needs fixing, too.
> 
> Haven't really done an in-depth audit, mostly just looking at
> where the asserts kick in..

Right - I just did a scan with cscope on the users of
XFS_ILOCK_SHARED, and those two were the only ones that stuck out
that weren't handled correctly....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/5] xfs: assert that we hold the ilock for extent map access

[Dave Chinner]

On Thu, Dec 05, 2013 at 07:58:35AM -0800, Christoph Hellwig wrote:
> Make sure that xfs_bmapi_read has the ilock held in some way, and that
> xfs_bmapi_write, xfs_bmapi_delay and xfs_iread_extents are called with
> the ilock held exclusively.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_bmap.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap.c	2013-11-29 14:25:12.172459195 +0100
> +++ xfs/fs/xfs/xfs_bmap.c	2013-12-05 10:03:28.243801633 +0100
> @@ -3997,6 +3997,7 @@ xfs_bmapi_read(
>  	ASSERT(*nmap >= 1);
>  	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK|XFS_BMAPI_ENTIRE|
>  			   XFS_BMAPI_IGSTATE)));
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
>  
>  	if (unlikely(XFS_TEST_ERROR(
>  	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> @@ -4191,6 +4192,7 @@ xfs_bmapi_delay(
>  	ASSERT(*nmap >= 1);
>  	ASSERT(*nmap <= XFS_BMAP_MAX_NMAP);
>  	ASSERT(!(flags & ~XFS_BMAPI_ENTIRE));
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
>  	if (unlikely(XFS_TEST_ERROR(
>  	    (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
> @@ -4484,6 +4486,7 @@ xfs_bmapi_write(
>  	ASSERT(tp != NULL);
>  	ASSERT(len > 0);
>  	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
>  	if (unlikely(XFS_TEST_ERROR(
>  	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> Index: xfs/fs/xfs/xfs_inode_fork.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_inode_fork.c	2013-12-05 09:57:24.347809100 +0100
> +++ xfs/fs/xfs/xfs_inode_fork.c	2013-12-05 09:59:04.767807039 +0100
> @@ -431,6 +431,8 @@ xfs_iread_extents(
>  	xfs_ifork_t	*ifp;
>  	xfs_extnum_t	nextents;
>  
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> +
>  	if (unlikely(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
>  		XFS_ERROR_REPORT("xfs_iread_extents", XFS_ERRLEVEL_LOW,
>  				 ip->i_mount);

Looks good, but can we add an assert to xfs_bunmapi() at the same
time just to cover all the public bmapi interfaces with locking
requirements?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/5] xfs: use xfs_ilock_map_shared in xfs_attr_get

[Christoph Hellwig]

On Fri, Dec 06, 2013 at 08:05:57AM +1100, Dave Chinner wrote:
> > Haven't really done an in-depth audit, mostly just looking at
> > where the asserts kick in..
> 
> Right - I just did a scan with cscope on the users of
> XFS_ILOCK_SHARED, and those two were the only ones that stuck out
> that weren't handled correctly....

With MAXPATHLEN at 1024 a symlink is at max 2 extents and thus never in
btree format, so I don't think we'll need it in readlink.  The attr
cases look real, though.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/5] xfs: use xfs_ilock_map_shared in xfs_attr_get

[Dave Chinner]

On Thu, Dec 05, 2013 at 01:10:55PM -0800, Christoph Hellwig wrote:
> On Fri, Dec 06, 2013 at 08:05:57AM +1100, Dave Chinner wrote:
> > > Haven't really done an in-depth audit, mostly just looking at
> > > where the asserts kick in..
> > 
> > Right - I just did a scan with cscope on the users of
> > XFS_ILOCK_SHARED, and those two were the only ones that stuck out
> > that weren't handled correctly....
> 
> With MAXPATHLEN at 1024 a symlink is at max 2 extents and thus never in
> btree format, so I don't think we'll need it in readlink.  The attr
> cases look real, though.

True, because the data fork will always have space for 3 extents
(#define MINDBTPTRS      3) and so it won't go out of line
regardless of the attribute fork.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/5] xfs: assert that we hold the ilock for extent map access

[Christoph Hellwig]

On Fri, Dec 06, 2013 at 08:10:47AM +1100, Dave Chinner wrote:
> Looks good, but can we add an assert to xfs_bunmapi() at the same
> time just to cover all the public bmapi interfaces with locking
> requirements?

Sure, will do.

Btw, I got another idea to sort this mess out a bit better:

 - add a new XFS_ILOCK_BMAP flag, and fold the bmap locking magic
   into xfs_ilock.
 - because the flag is now passed down we can assert that it is
   passed in xfs_bmapi_read and friends even if the extent list
   is already read in and thus improve coverage.

The downside is another two branches in the common ilock code path.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/5] xfs: assert that we hold the ilock for extent map access

[Dave Chinner]

On Thu, Dec 05, 2013 at 01:22:19PM -0800, Christoph Hellwig wrote:
> On Fri, Dec 06, 2013 at 08:10:47AM +1100, Dave Chinner wrote:
> > Looks good, but can we add an assert to xfs_bunmapi() at the same
> > time just to cover all the public bmapi interfaces with locking
> > requirements?
> 
> Sure, will do.
> 
> Btw, I got another idea to sort this mess out a bit better:
> 
>  - add a new XFS_ILOCK_BMAP flag, and fold the bmap locking magic
>    into xfs_ilock.
>  - because the flag is now passed down we can assert that it is
>    passed in xfs_bmapi_read and friends even if the extent list
>    is already read in and thus improve coverage.

Hmmm - I'm not sure I can see how that would work - the checks on
lock mode look at the lock directly, not at some other flag register
to indicate the locking context....

Are you thinking of expanding the code in xfs_isilocked() to handle
this as well? 

And what do we do with the unlock case, as we can't tell after the
fact from the inode state whether we locked shared or excl because
the extent list has now been read in....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs