* [PATCH 01/11] xfs: no need to lock the inode in xfs_find_handle
2013-12-06 20:30 [PATCH 00/11] extent list locking fixes V3 Christoph Hellwig
@ 2013-12-06 20:30 ` Christoph Hellwig
2013-12-08 22:31 ` Dave Chinner
2013-12-06 20:30 ` [PATCH 02/11] xfs: remove xfs_iunlock_map_shared Christoph Hellwig
` (9 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-06 20:30 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-fix-xfs_find_handle-locking --]
[-- Type: text/plain, Size: 940 bytes --]
Both the inode number and the generation do not change on a live inode.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: xfs/fs/xfs/xfs_ioctl.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ioctl.c 2013-12-06 19:16:33.819188640 +0100
+++ xfs/fs/xfs/xfs_ioctl.c 2013-12-06 19:17:04.087188019 +0100
@@ -112,15 +112,11 @@ xfs_find_handle(
memset(&handle.ha_fid, 0, sizeof(handle.ha_fid));
hsize = sizeof(xfs_fsid_t);
} else {
- int lock_mode;
-
- lock_mode = xfs_ilock_map_shared(ip);
handle.ha_fid.fid_len = sizeof(xfs_fid_t) -
sizeof(handle.ha_fid.fid_len);
handle.ha_fid.fid_pad = 0;
handle.ha_fid.fid_gen = ip->i_d.di_gen;
handle.ha_fid.fid_ino = ip->i_ino;
- xfs_iunlock_map_shared(ip, lock_mode);
hsize = XFS_HSIZE(handle);
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 01/11] xfs: no need to lock the inode in xfs_find_handle
2013-12-06 20:30 ` [PATCH 01/11] xfs: no need to lock the inode in xfs_find_handle Christoph Hellwig
@ 2013-12-08 22:31 ` Dave Chinner
0 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2013-12-08 22:31 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Fri, Dec 06, 2013 at 12:30:07PM -0800, Christoph Hellwig wrote:
> Both the inode number and the generation do not change on a live inode.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: xfs/fs/xfs/xfs_ioctl.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_ioctl.c 2013-12-06 19:16:33.819188640 +0100
> +++ xfs/fs/xfs/xfs_ioctl.c 2013-12-06 19:17:04.087188019 +0100
> @@ -112,15 +112,11 @@ xfs_find_handle(
> memset(&handle.ha_fid, 0, sizeof(handle.ha_fid));
> hsize = sizeof(xfs_fsid_t);
> } else {
> - int lock_mode;
> -
> - lock_mode = xfs_ilock_map_shared(ip);
> handle.ha_fid.fid_len = sizeof(xfs_fid_t) -
> sizeof(handle.ha_fid.fid_len);
> handle.ha_fid.fid_pad = 0;
> handle.ha_fid.fid_gen = ip->i_d.di_gen;
> handle.ha_fid.fid_ino = ip->i_ino;
> - xfs_iunlock_map_shared(ip, lock_mode);
>
> hsize = XFS_HSIZE(handle);
> }
Agreed.
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 02/11] xfs: remove xfs_iunlock_map_shared
2013-12-06 20:30 [PATCH 00/11] extent list locking fixes V3 Christoph Hellwig
2013-12-06 20:30 ` [PATCH 01/11] xfs: no need to lock the inode in xfs_find_handle Christoph Hellwig
@ 2013-12-06 20:30 ` Christoph Hellwig
2013-12-08 22:31 ` Dave Chinner
2013-12-06 20:30 ` [PATCH 03/11] xfs: rename xfs_ilock_map_shared Christoph Hellwig
` (8 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-06 20:30 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-remove-xfs_iunlock_map_shared --]
[-- Type: text/plain, Size: 3237 bytes --]
We can just use xfs_iunlock without any loss of clarity.
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-06 19:53:52.175142705 +0100
+++ xfs/fs/xfs/xfs_bmap_util.c 2013-12-06 19:57:22.371138392 +0100
@@ -737,7 +737,7 @@ xfs_getbmap(
out_free_map:
kmem_free(map);
out_unlock_ilock:
- xfs_iunlock_map_shared(ip, lock);
+ xfs_iunlock(ip, lock);
out_unlock_iolock:
xfs_iunlock(ip, XFS_IOLOCK_SHARED);
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c 2013-12-06 19:53:52.175142705 +0100
+++ xfs/fs/xfs/xfs_file.c 2013-12-06 19:57:22.371138392 +0100
@@ -1294,7 +1294,7 @@ out:
offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
out_unlock:
- xfs_iunlock_map_shared(ip, lock);
+ xfs_iunlock(ip, lock);
if (error)
return -error;
@@ -1402,7 +1402,7 @@ out:
offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
out_unlock:
- xfs_iunlock_map_shared(ip, lock);
+ xfs_iunlock(ip, lock);
if (error)
return -error;
Index: xfs/fs/xfs/xfs_inode.c
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.c 2013-12-06 19:53:52.175142705 +0100
+++ xfs/fs/xfs/xfs_inode.c 2013-12-06 19:57:33.199138169 +0100
@@ -88,8 +88,7 @@ xfs_get_extsz_hint(
* have been read in yet, and only lock the inode exclusively if they have not.
*
* The function returns a value which should be given to the corresponding
- * xfs_iunlock_map_shared(). This value is the mode in which the lock was
- * actually taken.
+ * xfs_iunlock() call.
*/
uint
xfs_ilock_map_shared(
@@ -110,18 +109,6 @@ xfs_ilock_map_shared(
}
/*
- * This is simply the unlock routine to go with xfs_ilock_map_shared().
- * All it does is call xfs_iunlock() with the given lock_mode.
- */
-void
-xfs_iunlock_map_shared(
- xfs_inode_t *ip,
- unsigned int lock_mode)
-{
- xfs_iunlock(ip, lock_mode);
-}
-
-/*
* The xfs inode contains 2 locks: a multi-reader lock called the
* i_iolock and a multi-reader lock called the i_lock. This routine
* allows either or both of the locks to be obtained.
@@ -590,7 +577,7 @@ xfs_lookup(
lock_mode = xfs_ilock_map_shared(dp);
error = xfs_dir_lookup(NULL, dp, name, &inum, ci_name);
- xfs_iunlock_map_shared(dp, lock_mode);
+ xfs_iunlock(dp, lock_mode);
if (error)
goto out;
Index: xfs/fs/xfs/xfs_inode.h
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.h 2013-12-06 19:53:52.175142705 +0100
+++ xfs/fs/xfs/xfs_inode.h 2013-12-06 19:57:22.371138392 +0100
@@ -338,7 +338,6 @@ void xfs_iunlock(xfs_inode_t *, uint);
void xfs_ilock_demote(xfs_inode_t *, uint);
int xfs_isilocked(xfs_inode_t *, uint);
uint xfs_ilock_map_shared(xfs_inode_t *);
-void xfs_iunlock_map_shared(xfs_inode_t *, uint);
int xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
xfs_nlink_t, xfs_dev_t, prid_t, int,
struct xfs_buf **, xfs_inode_t **);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH 03/11] xfs: rename xfs_ilock_map_shared
2013-12-06 20:30 [PATCH 00/11] extent list locking fixes V3 Christoph Hellwig
2013-12-06 20:30 ` [PATCH 01/11] xfs: no need to lock the inode in xfs_find_handle Christoph Hellwig
2013-12-06 20:30 ` [PATCH 02/11] xfs: remove xfs_iunlock_map_shared Christoph Hellwig
@ 2013-12-06 20:30 ` Christoph Hellwig
2013-12-08 22:33 ` Dave Chinner
2013-12-06 20:30 ` [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared Christoph Hellwig
` (7 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-06 20:30 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-rename-xfs_ilock_map_shared --]
[-- Type: text/plain, Size: 3919 bytes --]
Make it clear that we're only locking against the extent map on the data
fork. Also clean the function up a little bit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: xfs/fs/xfs/xfs_aops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_aops.c 2013-12-06 19:57:22.371138392 +0100
+++ xfs/fs/xfs/xfs_aops.c 2013-12-06 19:58:15.755137296 +0100
@@ -1217,7 +1217,7 @@ __xfs_get_blocks(
lockmode = XFS_ILOCK_EXCL;
xfs_ilock(ip, lockmode);
} else {
- lockmode = xfs_ilock_map_shared(ip);
+ lockmode = xfs_ilock_data_map_shared(ip);
}
ASSERT(offset <= mp->m_super->s_maxbytes);
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c 2013-12-06 19:57:22.371138392 +0100
+++ xfs/fs/xfs/xfs_file.c 2013-12-06 19:58:15.759137296 +0100
@@ -912,7 +912,7 @@ xfs_dir_open(
* If there are any blocks, read-ahead block 0 as we're almost
* certain to have the next operation be a read there.
*/
- mode = xfs_ilock_map_shared(ip);
+ mode = xfs_ilock_data_map_shared(ip);
if (ip->i_d.di_nextents > 0)
xfs_dir3_data_readahead(NULL, ip, 0, -1);
xfs_iunlock(ip, mode);
@@ -1215,7 +1215,7 @@ xfs_seek_data(
uint lock;
int error;
- lock = xfs_ilock_map_shared(ip);
+ lock = xfs_ilock_data_map_shared(ip);
isize = i_size_read(inode);
if (start >= isize) {
@@ -1319,7 +1319,7 @@ xfs_seek_hole(
if (XFS_FORCED_SHUTDOWN(mp))
return -XFS_ERROR(EIO);
- lock = xfs_ilock_map_shared(ip);
+ lock = xfs_ilock_data_map_shared(ip);
isize = i_size_read(inode);
if (start >= isize) {
Index: xfs/fs/xfs/xfs_inode.c
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.c 2013-12-06 19:57:33.199138169 +0100
+++ xfs/fs/xfs/xfs_inode.c 2013-12-06 19:58:41.667136764 +0100
@@ -91,20 +91,15 @@ xfs_get_extsz_hint(
* xfs_iunlock() call.
*/
uint
-xfs_ilock_map_shared(
- xfs_inode_t *ip)
+xfs_ilock_data_map_shared(
+ struct xfs_inode *ip)
{
- uint lock_mode;
+ uint lock_mode = XFS_ILOCK_SHARED;
- if ((ip->i_d.di_format == XFS_DINODE_FMT_BTREE) &&
- ((ip->i_df.if_flags & XFS_IFEXTENTS) == 0)) {
+ if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE &&
+ (ip->i_df.if_flags & XFS_IFEXTENTS) == 0)
lock_mode = XFS_ILOCK_EXCL;
- } else {
- lock_mode = XFS_ILOCK_SHARED;
- }
-
xfs_ilock(ip, lock_mode);
-
return lock_mode;
}
@@ -575,7 +570,7 @@ xfs_lookup(
if (XFS_FORCED_SHUTDOWN(dp->i_mount))
return XFS_ERROR(EIO);
- lock_mode = xfs_ilock_map_shared(dp);
+ lock_mode = xfs_ilock_data_map_shared(dp);
error = xfs_dir_lookup(NULL, dp, name, &inum, ci_name);
xfs_iunlock(dp, lock_mode);
Index: xfs/fs/xfs/xfs_inode.h
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.h 2013-12-06 19:57:22.371138392 +0100
+++ xfs/fs/xfs/xfs_inode.h 2013-12-06 19:58:15.759137296 +0100
@@ -337,7 +337,7 @@ int xfs_ilock_nowait(xfs_inode_t *, uin
void xfs_iunlock(xfs_inode_t *, uint);
void xfs_ilock_demote(xfs_inode_t *, uint);
int xfs_isilocked(xfs_inode_t *, uint);
-uint xfs_ilock_map_shared(xfs_inode_t *);
+uint xfs_ilock_data_map_shared(struct xfs_inode *);
int xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
xfs_nlink_t, xfs_dev_t, prid_t, int,
struct xfs_buf **, xfs_inode_t **);
Index: xfs/fs/xfs/xfs_bmap_util.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.c 2013-12-06 19:57:22.371138392 +0100
+++ xfs/fs/xfs/xfs_bmap_util.c 2013-12-06 19:58:15.759137296 +0100
@@ -632,7 +632,7 @@ xfs_getbmap(
*/
}
- lock = xfs_ilock_map_shared(ip);
+ lock = xfs_ilock_data_map_shared(ip);
/*
* Don't let nex be bigger than the number of extents
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 03/11] xfs: rename xfs_ilock_map_shared
2013-12-06 20:30 ` [PATCH 03/11] xfs: rename xfs_ilock_map_shared Christoph Hellwig
@ 2013-12-08 22:33 ` Dave Chinner
2013-12-09 7:03 ` Christoph Hellwig
0 siblings, 1 reply; 33+ messages in thread
From: Dave Chinner @ 2013-12-08 22:33 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Fri, Dec 06, 2013 at 12:30:09PM -0800, Christoph Hellwig wrote:
> Make it clear that we're only locking against the extent map on the data
> fork. Also clean the function up a little bit.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
.....
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_inode.c 2013-12-06 19:57:33.199138169 +0100
> +++ xfs/fs/xfs/xfs_inode.c 2013-12-06 19:58:41.667136764 +0100
> @@ -91,20 +91,15 @@ xfs_get_extsz_hint(
> * xfs_iunlock() call.
> */
> uint
> -xfs_ilock_map_shared(
> - xfs_inode_t *ip)
> +xfs_ilock_data_map_shared(
> + struct xfs_inode *ip)
> {
> - uint lock_mode;
> + uint lock_mode = XFS_ILOCK_SHARED;
>
> - if ((ip->i_d.di_format == XFS_DINODE_FMT_BTREE) &&
> - ((ip->i_df.if_flags & XFS_IFEXTENTS) == 0)) {
> + if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE &&
> + (ip->i_df.if_flags & XFS_IFEXTENTS) == 0)
> lock_mode = XFS_ILOCK_EXCL;
> - } else {
> - lock_mode = XFS_ILOCK_SHARED;
> - }
> -
> xfs_ilock(ip, lock_mode);
> -
> return lock_mode;
> }
While we are changing this, I think it makes sense to move it to
being a static inline function given how simple it is....
Otherwise it looks good.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 03/11] xfs: rename xfs_ilock_map_shared
2013-12-08 22:33 ` Dave Chinner
@ 2013-12-09 7:03 ` Christoph Hellwig
2013-12-09 7:24 ` Dave Chinner
0 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-09 7:03 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Mon, Dec 09, 2013 at 09:33:59AM +1100, Dave Chinner wrote:
> While we are changing this, I think it makes sense to move it to
> being a static inline function given how simple it is....
I actually tried that first, but with XFS_DINODE_FMT_BTREE in
fs/xfs/xfs_dinode.h, struct xfsicdinode in xfs_log_format.h
and the resulting dependencies it didn't seem workable. My inode
reshuffle might provide relief eventually.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 03/11] xfs: rename xfs_ilock_map_shared
2013-12-09 7:03 ` Christoph Hellwig
@ 2013-12-09 7:24 ` Dave Chinner
0 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2013-12-09 7:24 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Sun, Dec 08, 2013 at 11:03:16PM -0800, Christoph Hellwig wrote:
> On Mon, Dec 09, 2013 at 09:33:59AM +1100, Dave Chinner wrote:
> > While we are changing this, I think it makes sense to move it to
> > being a static inline function given how simple it is....
>
> I actually tried that first, but with XFS_DINODE_FMT_BTREE in
> fs/xfs/xfs_dinode.h, struct xfsicdinode in xfs_log_format.h
> and the resulting dependencies it didn't seem workable. My inode
> reshuffle might provide relief eventually.
Ok, makes sense to leave it like that for the moment.
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared
2013-12-06 20:30 [PATCH 00/11] extent list locking fixes V3 Christoph Hellwig
` (2 preceding siblings ...)
2013-12-06 20:30 ` [PATCH 03/11] xfs: rename xfs_ilock_map_shared Christoph Hellwig
@ 2013-12-06 20:30 ` Christoph Hellwig
2013-12-08 22:36 ` Dave Chinner
2013-12-06 20:30 ` [PATCH 05/11] xfs: reinstate the ilock in xfs_readdir Christoph Hellwig
` (6 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-06 20:30 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-add-xfs_ilock_attr_map_shared --]
[-- Type: text/plain, Size: 4861 bytes --]
Equivalent to xfs_ilock_data_map_shared, except for the attribute fork.
Make xfs_getbmap use it if called for the attribute fork instead of
xfs_ilock_data_map_shared.
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-06 19:58:15.759137296 +0100
+++ xfs/fs/xfs/xfs_bmap_util.c 2013-12-06 20:02:23.291132216 +0100
@@ -617,22 +617,27 @@ xfs_getbmap(
return XFS_ERROR(ENOMEM);
xfs_ilock(ip, XFS_IOLOCK_SHARED);
- if (whichfork == XFS_DATA_FORK && !(iflags & BMV_IF_DELALLOC)) {
- if (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size) {
+ if (whichfork == XFS_DATA_FORK) {
+ if (!(iflags & BMV_IF_DELALLOC) &&
+ (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size)) {
error = -filemap_write_and_wait(VFS_I(ip)->i_mapping);
if (error)
goto out_unlock_iolock;
+
+ /*
+ * Even after flushing the inode, there can still be
+ * delalloc blocks on the inode beyond EOF due to
+ * speculative reallocation. These are not removed
+ * until the release function is called or the inode
+ * is inactivated. Hence we cannot assert here that
+ * ip->i_delayed_blks == 0.
+ */
}
- /*
- * even after flushing the inode, there can still be delalloc
- * blocks on the inode beyond EOF due to speculative
- * preallocation. These are not removed until the release
- * function is called or the inode is inactivated. Hence we
- * cannot assert here that ip->i_delayed_blks == 0.
- */
- }
- lock = xfs_ilock_data_map_shared(ip);
+ lock = xfs_ilock_data_map_shared(ip);
+ } else {
+ lock = xfs_ilock_attr_map_shared(ip);
+ }
/*
* Don't let nex be bigger than the number of extents
Index: xfs/fs/xfs/xfs_inode.c
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.c 2013-12-06 19:58:41.667136764 +0100
+++ xfs/fs/xfs/xfs_inode.c 2013-12-06 20:00:44.535134243 +0100
@@ -77,17 +77,18 @@ xfs_get_extsz_hint(
}
/*
- * This is a wrapper routine around the xfs_ilock() routine used to centralize
- * some grungy code. It is used in places that wish to lock the inode solely
- * for reading the extents. The reason these places can't just call
- * xfs_ilock(SHARED) is that the inode lock also guards to bringing in of the
- * extents from disk for a file in b-tree format. If the inode is in b-tree
- * format, then we need to lock the inode exclusively until the extents are read
- * in. Locking it exclusively all the time would limit our parallelism
- * unnecessarily, though. What we do instead is check to see if the extents
- * have been read in yet, and only lock the inode exclusively if they have not.
+ * These two are wrapper routines around the xfs_ilock() routine used to
+ * centralize some grungy code. They are used in places that wish to lock the
+ * inode solely for reading the extents. The reason these places can't just
+ * call xfs_ilock(ip, XFS_ILOCK_SHARED) is that the inode lock also guards to
+ * bringing in of the extents from disk for a file in b-tree format. If the
+ * inode is in b-tree format, then we need to lock the inode exclusively until
+ * the extents are read in. Locking it exclusively all the time would limit
+ * our parallelism unnecessarily, though. What we do instead is check to see
+ * if the extents have been read in yet, and only lock the inode exclusively
+ * if they have not.
*
- * The function returns a value which should be given to the corresponding
+ * The functions return a value which should be given to the corresponding
* xfs_iunlock() call.
*/
uint
@@ -101,6 +102,19 @@ xfs_ilock_data_map_shared(
lock_mode = XFS_ILOCK_EXCL;
xfs_ilock(ip, lock_mode);
return lock_mode;
+}
+
+uint
+xfs_ilock_attr_map_shared(
+ struct xfs_inode *ip)
+{
+ uint lock_mode = XFS_ILOCK_SHARED;
+
+ if (ip->i_d.di_aformat == XFS_DINODE_FMT_BTREE &&
+ (ip->i_afp->if_flags & XFS_IFEXTENTS) == 0)
+ lock_mode = XFS_ILOCK_EXCL;
+ xfs_ilock(ip, lock_mode);
+ return lock_mode;
}
/*
Index: xfs/fs/xfs/xfs_inode.h
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.h 2013-12-06 19:58:15.759137296 +0100
+++ xfs/fs/xfs/xfs_inode.h 2013-12-06 20:00:50.955134111 +0100
@@ -338,6 +338,7 @@ void xfs_iunlock(xfs_inode_t *, uint);
void xfs_ilock_demote(xfs_inode_t *, uint);
int xfs_isilocked(xfs_inode_t *, uint);
uint xfs_ilock_data_map_shared(struct xfs_inode *);
+uint xfs_ilock_attr_map_shared(struct xfs_inode *);
int xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
xfs_nlink_t, xfs_dev_t, prid_t, int,
struct xfs_buf **, xfs_inode_t **);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared
2013-12-06 20:30 ` [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared Christoph Hellwig
@ 2013-12-08 22:36 ` Dave Chinner
2013-12-09 18:16 ` Christoph Hellwig
0 siblings, 1 reply; 33+ messages in thread
From: Dave Chinner @ 2013-12-08 22:36 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Fri, Dec 06, 2013 at 12:30:10PM -0800, Christoph Hellwig wrote:
> Equivalent to xfs_ilock_data_map_shared, except for the attribute fork.
>
> Make xfs_getbmap use it if called for the attribute fork instead of
> xfs_ilock_data_map_shared.
>
> 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-06 19:58:15.759137296 +0100
> +++ xfs/fs/xfs/xfs_bmap_util.c 2013-12-06 20:02:23.291132216 +0100
> @@ -617,22 +617,27 @@ xfs_getbmap(
> return XFS_ERROR(ENOMEM);
>
> xfs_ilock(ip, XFS_IOLOCK_SHARED);
> - if (whichfork == XFS_DATA_FORK && !(iflags & BMV_IF_DELALLOC)) {
> - if (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size) {
> + if (whichfork == XFS_DATA_FORK) {
> + if (!(iflags & BMV_IF_DELALLOC) &&
> + (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size)) {
> error = -filemap_write_and_wait(VFS_I(ip)->i_mapping);
> if (error)
> goto out_unlock_iolock;
> +
> + /*
> + * Even after flushing the inode, there can still be
> + * delalloc blocks on the inode beyond EOF due to
> + * speculative reallocation. These are not removed
"speculative preallocation"
> Index: xfs/fs/xfs/xfs_inode.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_inode.c 2013-12-06 19:58:41.667136764 +0100
> +++ xfs/fs/xfs/xfs_inode.c 2013-12-06 20:00:44.535134243 +0100
> @@ -77,17 +77,18 @@ xfs_get_extsz_hint(
> }
>
> /*
> - * This is a wrapper routine around the xfs_ilock() routine used to centralize
> - * some grungy code. It is used in places that wish to lock the inode solely
> - * for reading the extents. The reason these places can't just call
> - * xfs_ilock(SHARED) is that the inode lock also guards to bringing in of the
> - * extents from disk for a file in b-tree format. If the inode is in b-tree
> - * format, then we need to lock the inode exclusively until the extents are read
> - * in. Locking it exclusively all the time would limit our parallelism
> - * unnecessarily, though. What we do instead is check to see if the extents
> - * have been read in yet, and only lock the inode exclusively if they have not.
> + * These two are wrapper routines around the xfs_ilock() routine used to
> + * centralize some grungy code. They are used in places that wish to lock the
> + * inode solely for reading the extents. The reason these places can't just
> + * call xfs_ilock(ip, XFS_ILOCK_SHARED) is that the inode lock also guards to
> + * bringing in of the extents from disk for a file in b-tree format. If the
> + * inode is in b-tree format, then we need to lock the inode exclusively until
> + * the extents are read in. Locking it exclusively all the time would limit
> + * our parallelism unnecessarily, though. What we do instead is check to see
> + * if the extents have been read in yet, and only lock the inode exclusively
> + * if they have not.
> *
> - * The function returns a value which should be given to the corresponding
> + * The functions return a value which should be given to the corresponding
> * xfs_iunlock() call.
> */
> uint
> @@ -101,6 +102,19 @@ xfs_ilock_data_map_shared(
> lock_mode = XFS_ILOCK_EXCL;
> xfs_ilock(ip, lock_mode);
> return lock_mode;
> +}
> +
> +uint
> +xfs_ilock_attr_map_shared(
> + struct xfs_inode *ip)
> +{
> + uint lock_mode = XFS_ILOCK_SHARED;
> +
> + if (ip->i_d.di_aformat == XFS_DINODE_FMT_BTREE &&
> + (ip->i_afp->if_flags & XFS_IFEXTENTS) == 0)
> + lock_mode = XFS_ILOCK_EXCL;
> + xfs_ilock(ip, lock_mode);
> + return lock_mode;
> }
Again, I think a static inline is appropriate here...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared
2013-12-08 22:36 ` Dave Chinner
@ 2013-12-09 18:16 ` Christoph Hellwig
2013-12-09 18:52 ` Ben Myers
2013-12-09 22:24 ` Dave Chinner
0 siblings, 2 replies; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-09 18:16 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Mon, Dec 09, 2013 at 09:36:10AM +1100, Dave Chinner wrote:
> > + /*
> > + * Even after flushing the inode, there can still be
> > + * delalloc blocks on the inode beyond EOF due to
> > + * speculative reallocation. These are not removed
>
> "speculative preallocation"
I just re-indented the comment, the wording is the original one. Maybe
we'll get a commit that cares enought to fix it on the fly..
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared
2013-12-09 18:16 ` Christoph Hellwig
@ 2013-12-09 18:52 ` Ben Myers
2013-12-09 22:24 ` Dave Chinner
1 sibling, 0 replies; 33+ messages in thread
From: Ben Myers @ 2013-12-09 18:52 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Dec 09, 2013 at 10:16:12AM -0800, Christoph Hellwig wrote:
> On Mon, Dec 09, 2013 at 09:36:10AM +1100, Dave Chinner wrote:
> > > + /*
> > > + * Even after flushing the inode, there can still be
> > > + * delalloc blocks on the inode beyond EOF due to
> > > + * speculative reallocation. These are not removed
> >
> > "speculative preallocation"
>
> I just re-indented the comment, the wording is the original one. Maybe
> we'll get a commit that cares enought to fix it on the fly..
Sure...
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared
2013-12-09 18:16 ` Christoph Hellwig
2013-12-09 18:52 ` Ben Myers
@ 2013-12-09 22:24 ` Dave Chinner
2013-12-09 22:30 ` Ben Myers
2013-12-10 16:13 ` Christoph Hellwig
1 sibling, 2 replies; 33+ messages in thread
From: Dave Chinner @ 2013-12-09 22:24 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Dec 09, 2013 at 10:16:12AM -0800, Christoph Hellwig wrote:
> On Mon, Dec 09, 2013 at 09:36:10AM +1100, Dave Chinner wrote:
> > > + /*
> > > + * Even after flushing the inode, there can still be
> > > + * delalloc blocks on the inode beyond EOF due to
> > > + * speculative reallocation. These are not removed
> >
> > "speculative preallocation"
>
> I just re-indented the comment, the wording is the original one. Maybe
> we'll get a commit that cares enought to fix it on the fly..
Actually, I checked that before commenting on it - the original
comment is correct:
626 /*
627 * even after flushing the inode, there can still be delalloc
628 * blocks on the inode beyond EOF due to speculative
629 * preallocation. These are not removed until the release
630 * function is called or the inode is inactivated. Hence we
631 * cannot assert here that ip->i_delayed_blks == 0.
632 */
I wouldn't have pointed at it if the original code had that
problem... :/
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared
2013-12-09 22:24 ` Dave Chinner
@ 2013-12-09 22:30 ` Ben Myers
2013-12-10 16:13 ` Christoph Hellwig
1 sibling, 0 replies; 33+ messages in thread
From: Ben Myers @ 2013-12-09 22:30 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
Dave,
On Tue, Dec 10, 2013 at 09:24:33AM +1100, Dave Chinner wrote:
> On Mon, Dec 09, 2013 at 10:16:12AM -0800, Christoph Hellwig wrote:
> > On Mon, Dec 09, 2013 at 09:36:10AM +1100, Dave Chinner wrote:
> > > > + /*
> > > > + * Even after flushing the inode, there can still be
> > > > + * delalloc blocks on the inode beyond EOF due to
> > > > + * speculative reallocation. These are not removed
> > >
> > > "speculative preallocation"
> >
> > I just re-indented the comment, the wording is the original one. Maybe
> > we'll get a commit that cares enought to fix it on the fly..
>
> Actually, I checked that before commenting on it - the original
> comment is correct:
>
> 626 /*
> 627 * even after flushing the inode, there can still be delalloc
> 628 * blocks on the inode beyond EOF due to speculative
> 629 * preallocation. These are not removed until the release
> 630 * function is called or the inode is inactivated. Hence we
> 631 * cannot assert here that ip->i_delayed_blks == 0.
> 632 */
>
> I wouldn't have pointed at it if the original code had that
> problem... :/
If you want to extend your Reviewed-by: on this patch I will clean up the
spelling mistake and pull in this series.
-Ben
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared
2013-12-09 22:24 ` Dave Chinner
2013-12-09 22:30 ` Ben Myers
@ 2013-12-10 16:13 ` Christoph Hellwig
2013-12-17 17:33 ` Ben Myers
1 sibling, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-10 16:13 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Tue, Dec 10, 2013 at 09:24:33AM +1100, Dave Chinner wrote:
> I wouldn't have pointed at it if the original code had that
> problem... :/
Strange. I'd swear I haven't reworded anything. I'll fix it back up.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared
2013-12-10 16:13 ` Christoph Hellwig
@ 2013-12-17 17:33 ` Ben Myers
2013-12-18 10:14 ` [PATCH 04/11 v2] " Christoph Hellwig
0 siblings, 1 reply; 33+ messages in thread
From: Ben Myers @ 2013-12-17 17:33 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
Hey Christoph,
On Tue, Dec 10, 2013 at 08:13:49AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 10, 2013 at 09:24:33AM +1100, Dave Chinner wrote:
> > I wouldn't have pointed at it if the original code had that
> > problem... :/
>
> Strange. I'd swear I haven't reworded anything. I'll fix it back up.
I'd like to pull this series in. Are you still planning on reposting this patch?
Thanks,
Ben
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 04/11 v2] xfs: add xfs_ilock_attr_map_shared
2013-12-17 17:33 ` Ben Myers
@ 2013-12-18 10:14 ` Christoph Hellwig
2013-12-18 21:47 ` Ben Myers
0 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-18 10:14 UTC (permalink / raw)
To: Ben Myers; +Cc: xfs
Equivalent to xfs_ilock_data_map_shared, except for the attribute fork.
Make xfs_getbmap use it if called for the attribute fork instead of
xfs_ilock_data_map_shared.
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-18 11:14:52.587953376 +0100
+++ xfs/fs/xfs/xfs_bmap_util.c 2013-12-18 11:15:29.367952621 +0100
@@ -617,22 +617,27 @@ xfs_getbmap(
return XFS_ERROR(ENOMEM);
xfs_ilock(ip, XFS_IOLOCK_SHARED);
- if (whichfork == XFS_DATA_FORK && !(iflags & BMV_IF_DELALLOC)) {
- if (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size) {
+ if (whichfork == XFS_DATA_FORK) {
+ if (!(iflags & BMV_IF_DELALLOC) &&
+ (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size)) {
error = -filemap_write_and_wait(VFS_I(ip)->i_mapping);
if (error)
goto out_unlock_iolock;
+
+ /*
+ * Even after flushing the inode, there can still be
+ * delalloc blocks on the inode beyond EOF due to
+ * speculative preallocation. These are not removed
+ * until the release function is called or the inode
+ * is inactivated. Hence we cannot assert here that
+ * ip->i_delayed_blks == 0.
+ */
}
- /*
- * even after flushing the inode, there can still be delalloc
- * blocks on the inode beyond EOF due to speculative
- * preallocation. These are not removed until the release
- * function is called or the inode is inactivated. Hence we
- * cannot assert here that ip->i_delayed_blks == 0.
- */
- }
- lock = xfs_ilock_data_map_shared(ip);
+ lock = xfs_ilock_data_map_shared(ip);
+ } else {
+ lock = xfs_ilock_attr_map_shared(ip);
+ }
/*
* Don't let nex be bigger than the number of extents
Index: xfs/fs/xfs/xfs_inode.c
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.c 2013-12-18 11:14:52.587953376 +0100
+++ xfs/fs/xfs/xfs_inode.c 2013-12-18 11:14:53.599953355 +0100
@@ -77,17 +77,18 @@ xfs_get_extsz_hint(
}
/*
- * This is a wrapper routine around the xfs_ilock() routine used to centralize
- * some grungy code. It is used in places that wish to lock the inode solely
- * for reading the extents. The reason these places can't just call
- * xfs_ilock(SHARED) is that the inode lock also guards to bringing in of the
- * extents from disk for a file in b-tree format. If the inode is in b-tree
- * format, then we need to lock the inode exclusively until the extents are read
- * in. Locking it exclusively all the time would limit our parallelism
- * unnecessarily, though. What we do instead is check to see if the extents
- * have been read in yet, and only lock the inode exclusively if they have not.
+ * These two are wrapper routines around the xfs_ilock() routine used to
+ * centralize some grungy code. They are used in places that wish to lock the
+ * inode solely for reading the extents. The reason these places can't just
+ * call xfs_ilock(ip, XFS_ILOCK_SHARED) is that the inode lock also guards to
+ * bringing in of the extents from disk for a file in b-tree format. If the
+ * inode is in b-tree format, then we need to lock the inode exclusively until
+ * the extents are read in. Locking it exclusively all the time would limit
+ * our parallelism unnecessarily, though. What we do instead is check to see
+ * if the extents have been read in yet, and only lock the inode exclusively
+ * if they have not.
*
- * The function returns a value which should be given to the corresponding
+ * The functions return a value which should be given to the corresponding
* xfs_iunlock() call.
*/
uint
@@ -101,6 +102,19 @@ xfs_ilock_data_map_shared(
lock_mode = XFS_ILOCK_EXCL;
xfs_ilock(ip, lock_mode);
return lock_mode;
+}
+
+uint
+xfs_ilock_attr_map_shared(
+ struct xfs_inode *ip)
+{
+ uint lock_mode = XFS_ILOCK_SHARED;
+
+ if (ip->i_d.di_aformat == XFS_DINODE_FMT_BTREE &&
+ (ip->i_afp->if_flags & XFS_IFEXTENTS) == 0)
+ lock_mode = XFS_ILOCK_EXCL;
+ xfs_ilock(ip, lock_mode);
+ return lock_mode;
}
/*
Index: xfs/fs/xfs/xfs_inode.h
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.h 2013-12-18 11:14:52.587953376 +0100
+++ xfs/fs/xfs/xfs_inode.h 2013-12-18 11:14:53.599953355 +0100
@@ -338,6 +338,7 @@ void xfs_iunlock(xfs_inode_t *, uint);
void xfs_ilock_demote(xfs_inode_t *, uint);
int xfs_isilocked(xfs_inode_t *, uint);
uint xfs_ilock_data_map_shared(struct xfs_inode *);
+uint xfs_ilock_attr_map_shared(struct xfs_inode *);
int xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
xfs_nlink_t, xfs_dev_t, prid_t, int,
struct xfs_buf **, xfs_inode_t **);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 04/11 v2] xfs: add xfs_ilock_attr_map_shared
2013-12-18 10:14 ` [PATCH 04/11 v2] " Christoph Hellwig
@ 2013-12-18 21:47 ` Ben Myers
0 siblings, 0 replies; 33+ messages in thread
From: Ben Myers @ 2013-12-18 21:47 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Wed, Dec 18, 2013 at 02:14:39AM -0800, Christoph Hellwig wrote:
> Equivalent to xfs_ilock_data_map_shared, except for the attribute fork.
>
> Make xfs_getbmap use it if called for the attribute fork instead of
> xfs_ilock_data_map_shared.
>
> 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-18 11:14:52.587953376 +0100
> +++ xfs/fs/xfs/xfs_bmap_util.c 2013-12-18 11:15:29.367952621 +0100
> @@ -617,22 +617,27 @@ xfs_getbmap(
> return XFS_ERROR(ENOMEM);
>
> xfs_ilock(ip, XFS_IOLOCK_SHARED);
> - if (whichfork == XFS_DATA_FORK && !(iflags & BMV_IF_DELALLOC)) {
> - if (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size) {
> + if (whichfork == XFS_DATA_FORK) {
> + if (!(iflags & BMV_IF_DELALLOC) &&
> + (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size)) {
> error = -filemap_write_and_wait(VFS_I(ip)->i_mapping);
> if (error)
> goto out_unlock_iolock;
> +
> + /*
> + * Even after flushing the inode, there can still be
> + * delalloc blocks on the inode beyond EOF due to
> + * speculative preallocation. These are not removed
This one has a 'p'.
Reviewed-by: Ben Myers <bpm@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 05/11] xfs: reinstate the ilock in xfs_readdir
2013-12-06 20:30 [PATCH 00/11] extent list locking fixes V3 Christoph Hellwig
` (3 preceding siblings ...)
2013-12-06 20:30 ` [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared Christoph Hellwig
@ 2013-12-06 20:30 ` Christoph Hellwig
2013-12-08 22:36 ` Dave Chinner
2013-12-06 20:30 ` [PATCH 06/11] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes Christoph Hellwig
` (5 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-06 20:30 UTC (permalink / raw)
To: xfs; +Cc: Ben Myers
[-- Attachment #1: iolock.diff --]
[-- Type: text/plain, Size: 1674 bytes --]
From: Ben Myers <bpm@sgi.com>
Although it was removed in commit 051e7cd44ab8, ilock needs to be taken in
xfs_readdir because we might have to read the extent list in from disk. This
keeps other threads from reading from or writing to the extent list while it is
being read in and is still in a transitional state.
This has been associated with "Access to block zero" messages on directories
with large numbers of extents resulting from excessive filesytem fragmentation,
as well as extent list corruption. Unfortunately no test case at this point.
Signed-off-by: Ben Myers <bpm@sgi.com>
---
fs/xfs/xfs_dir2_readdir.c | 4 ++++
1 file changed, 4 insertions(+)
Index: xfs/fs/xfs/xfs_dir2_readdir.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dir2_readdir.c 2013-12-06 17:20:27.695331596 +0100
+++ xfs/fs/xfs/xfs_dir2_readdir.c 2013-12-06 19:38:08.375162074 +0100
@@ -674,6 +674,7 @@ xfs_readdir(
{
int rval; /* return value */
int v; /* type-checking value */
+ uint lock_mode;
trace_xfs_readdir(dp);
@@ -683,6 +684,7 @@ xfs_readdir(
ASSERT(S_ISDIR(dp->i_d.di_mode));
XFS_STATS_INC(xs_dir_getdents);
+ lock_mode = xfs_ilock_data_map_shared(dp);
if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL)
rval = xfs_dir2_sf_getdents(dp, ctx);
else if ((rval = xfs_dir2_isblock(NULL, dp, &v)))
@@ -691,5 +693,7 @@ xfs_readdir(
rval = xfs_dir2_block_getdents(dp, ctx);
else
rval = xfs_dir2_leaf_getdents(dp, ctx, bufsize);
+ xfs_iunlock(dp, lock_mode);
+
return rval;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 05/11] xfs: reinstate the ilock in xfs_readdir
2013-12-06 20:30 ` [PATCH 05/11] xfs: reinstate the ilock in xfs_readdir Christoph Hellwig
@ 2013-12-08 22:36 ` Dave Chinner
0 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2013-12-08 22:36 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Ben Myers, xfs
On Fri, Dec 06, 2013 at 12:30:11PM -0800, Christoph Hellwig wrote:
> From: Ben Myers <bpm@sgi.com>
>
> Although it was removed in commit 051e7cd44ab8, ilock needs to be taken in
> xfs_readdir because we might have to read the extent list in from disk. This
> keeps other threads from reading from or writing to the extent list while it is
> being read in and is still in a transitional state.
>
> This has been associated with "Access to block zero" messages on directories
> with large numbers of extents resulting from excessive filesytem fragmentation,
> as well as extent list corruption. Unfortunately no test case at this point.
>
> Signed-off-by: Ben Myers <bpm@sgi.com>
Looks good.
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 06/11] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes
2013-12-06 20:30 [PATCH 00/11] extent list locking fixes V3 Christoph Hellwig
` (4 preceding siblings ...)
2013-12-06 20:30 ` [PATCH 05/11] xfs: reinstate the ilock in xfs_readdir Christoph Hellwig
@ 2013-12-06 20:30 ` Christoph Hellwig
2013-12-08 22:36 ` Dave Chinner
2013-12-06 20:30 ` [PATCH 07/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqtobp Christoph Hellwig
` (4 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-06 20:30 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs_zero_remaining_bytes-locking --]
[-- Type: text/plain, Size: 851 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-06 19:37:36.715162723 +0100
+++ xfs/fs/xfs/xfs_bmap_util.c 2013-12-06 19:38:45.643161309 +0100
@@ -1175,9 +1175,15 @@ xfs_zero_remaining_bytes(
xfs_buf_unlock(bp);
for (offset = startoff; offset <= endoff; offset = lastoffset + 1) {
+ uint lock_mode;
+
offset_fsb = XFS_B_TO_FSBT(mp, offset);
nimap = 1;
+
+ lock_mode = xfs_ilock_data_map_shared(ip);
error = xfs_bmapi_read(ip, offset_fsb, 1, &imap, &nimap, 0);
+ xfs_iunlock(ip, lock_mode);
+
if (error || nimap < 1)
break;
ASSERT(imap.br_blockcount >= 1);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 06/11] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes
2013-12-06 20:30 ` [PATCH 06/11] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes Christoph Hellwig
@ 2013-12-08 22:36 ` Dave Chinner
0 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2013-12-08 22:36 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Fri, Dec 06, 2013 at 12:30:12PM -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-06 19:37:36.715162723 +0100
> +++ xfs/fs/xfs/xfs_bmap_util.c 2013-12-06 19:38:45.643161309 +0100
> @@ -1175,9 +1175,15 @@ xfs_zero_remaining_bytes(
> xfs_buf_unlock(bp);
>
> for (offset = startoff; offset <= endoff; offset = lastoffset + 1) {
> + uint lock_mode;
> +
> offset_fsb = XFS_B_TO_FSBT(mp, offset);
> nimap = 1;
> +
> + lock_mode = xfs_ilock_data_map_shared(ip);
> error = xfs_bmapi_read(ip, offset_fsb, 1, &imap, &nimap, 0);
> + xfs_iunlock(ip, lock_mode);
> +
> if (error || nimap < 1)
> break;
> ASSERT(imap.br_blockcount >= 1);
Looks good.
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 07/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqtobp
2013-12-06 20:30 [PATCH 00/11] extent list locking fixes V3 Christoph Hellwig
` (5 preceding siblings ...)
2013-12-06 20:30 ` [PATCH 06/11] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes Christoph Hellwig
@ 2013-12-06 20:30 ` Christoph Hellwig
2013-12-08 22:38 ` Dave Chinner
2013-12-06 20:30 ` [PATCH 08/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqiterate Christoph Hellwig
` (3 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-06 20:30 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-quota-locking --]
[-- Type: text/plain, Size: 1400 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-12-06 17:20:27.571331599 +0100
+++ xfs/fs/xfs/xfs_dquot.c 2013-12-06 19:39:42.307160146 +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_data_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(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(quotip, lock_mode);
if (error)
return error;
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 07/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqtobp
2013-12-06 20:30 ` [PATCH 07/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqtobp Christoph Hellwig
@ 2013-12-08 22:38 ` Dave Chinner
0 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2013-12-08 22:38 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Fri, Dec 06, 2013 at 12:30:13PM -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-12-06 17:20:27.571331599 +0100
> +++ xfs/fs/xfs/xfs_dquot.c 2013-12-06 19:39:42.307160146 +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_data_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(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(quotip, lock_mode);
> if (error)
> return error;
Looks fine - it doesn't change the race condition w/ dquot creation
at all, so we can ignore that for now...
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 08/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqiterate
2013-12-06 20:30 [PATCH 00/11] extent list locking fixes V3 Christoph Hellwig
` (6 preceding siblings ...)
2013-12-06 20:30 ` [PATCH 07/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqtobp Christoph Hellwig
@ 2013-12-06 20:30 ` Christoph Hellwig
2013-12-08 22:38 ` Dave Chinner
2013-12-06 20:30 ` [PATCH 09/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_get Christoph Hellwig
` (2 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-06 20:30 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-quota-locking-2 --]
[-- Type: text/plain, Size: 1136 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-12-06 17:20:27.491331600 +0100
+++ xfs/fs/xfs/xfs_qm.c 2013-12-06 19:39:27.251160455 +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_data_map_shared(qip);
error = xfs_bmapi_read(qip, lblkno, maxlblkcnt - lblkno,
map, &nmaps, 0);
- xfs_iunlock(qip, XFS_ILOCK_SHARED);
+ xfs_iunlock(qip, lock_mode);
if (error)
break;
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 08/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqiterate
2013-12-06 20:30 ` [PATCH 08/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqiterate Christoph Hellwig
@ 2013-12-08 22:38 ` Dave Chinner
0 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2013-12-08 22:38 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Fri, Dec 06, 2013 at 12:30:14PM -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-12-06 17:20:27.491331600 +0100
> +++ xfs/fs/xfs/xfs_qm.c 2013-12-06 19:39:27.251160455 +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_data_map_shared(qip);
> error = xfs_bmapi_read(qip, lblkno, maxlblkcnt - lblkno,
> map, &nmaps, 0);
> - xfs_iunlock(qip, XFS_ILOCK_SHARED);
> + xfs_iunlock(qip, lock_mode);
> if (error)
> break;
looks good.
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 09/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_get
2013-12-06 20:30 [PATCH 00/11] extent list locking fixes V3 Christoph Hellwig
` (7 preceding siblings ...)
2013-12-06 20:30 ` [PATCH 08/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqiterate Christoph Hellwig
@ 2013-12-06 20:30 ` Christoph Hellwig
2013-12-08 22:39 ` Dave Chinner
2013-12-06 20:30 ` [PATCH 10/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_list_int Christoph Hellwig
2013-12-06 20:30 ` [PATCH 11/11] xfs: assert that we hold the ilock for extent map access Christoph Hellwig
10 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-06 20:30 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-attr-locking --]
[-- Type: text/plain, Size: 946 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_attr.c
===================================================================
--- xfs.orig/fs/xfs/xfs_attr.c 2013-12-06 17:20:27.447331601 +0100
+++ xfs/fs/xfs/xfs_attr.c 2013-12-06 19:41:05.119158446 +0100
@@ -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_attr_map_shared(ip);
error = xfs_attr_get_int(ip, &xname, value, valuelenp, flags);
- xfs_iunlock(ip, XFS_ILOCK_SHARED);
+ xfs_iunlock(ip, lock_mode);
return(error);
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 09/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_get
2013-12-06 20:30 ` [PATCH 09/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_get Christoph Hellwig
@ 2013-12-08 22:39 ` Dave Chinner
0 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2013-12-08 22:39 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Fri, Dec 06, 2013 at 12:30:15PM -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_attr.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_attr.c 2013-12-06 17:20:27.447331601 +0100
> +++ xfs/fs/xfs/xfs_attr.c 2013-12-06 19:41:05.119158446 +0100
> @@ -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_attr_map_shared(ip);
> error = xfs_attr_get_int(ip, &xname, value, valuelenp, flags);
> - xfs_iunlock(ip, XFS_ILOCK_SHARED);
> + xfs_iunlock(ip, lock_mode);
> return(error);
> }
Yup, that should fix the assert problem. :)
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 10/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_list_int
2013-12-06 20:30 [PATCH 00/11] extent list locking fixes V3 Christoph Hellwig
` (8 preceding siblings ...)
2013-12-06 20:30 ` [PATCH 09/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_get Christoph Hellwig
@ 2013-12-06 20:30 ` Christoph Hellwig
2013-12-08 22:39 ` Dave Chinner
2013-12-06 20:30 ` [PATCH 11/11] xfs: assert that we hold the ilock for extent map access Christoph Hellwig
10 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-06 20:30 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-attr-locking-2 --]
[-- Type: text/plain, Size: 1184 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_attr_list.c
===================================================================
--- xfs.orig/fs/xfs/xfs_attr_list.c 2013-12-06 17:20:27.371331603 +0100
+++ xfs/fs/xfs/xfs_attr_list.c 2013-12-06 19:41:42.779157674 +0100
@@ -507,17 +507,17 @@ xfs_attr_list_int(
{
int error;
xfs_inode_t *dp = context->dp;
+ uint lock_mode;
XFS_STATS_INC(xs_attr_list);
if (XFS_FORCED_SHUTDOWN(dp->i_mount))
return EIO;
- xfs_ilock(dp, XFS_ILOCK_SHARED);
-
/*
* Decide on what work routines to call based on the inode size.
*/
+ lock_mode = xfs_ilock_attr_map_shared(dp);
if (!xfs_inode_hasattr(dp)) {
error = 0;
} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
@@ -527,9 +527,7 @@ xfs_attr_list_int(
} else {
error = xfs_attr_node_list(context);
}
-
- xfs_iunlock(dp, XFS_ILOCK_SHARED);
-
+ xfs_iunlock(dp, lock_mode);
return error;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 10/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_list_int
2013-12-06 20:30 ` [PATCH 10/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_list_int Christoph Hellwig
@ 2013-12-08 22:39 ` Dave Chinner
0 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2013-12-08 22:39 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Fri, Dec 06, 2013 at 12:30:16PM -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_attr_list.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_attr_list.c 2013-12-06 17:20:27.371331603 +0100
> +++ xfs/fs/xfs/xfs_attr_list.c 2013-12-06 19:41:42.779157674 +0100
> @@ -507,17 +507,17 @@ xfs_attr_list_int(
> {
> int error;
> xfs_inode_t *dp = context->dp;
> + uint lock_mode;
>
> XFS_STATS_INC(xs_attr_list);
>
> if (XFS_FORCED_SHUTDOWN(dp->i_mount))
> return EIO;
>
> - xfs_ilock(dp, XFS_ILOCK_SHARED);
> -
> /*
> * Decide on what work routines to call based on the inode size.
> */
> + lock_mode = xfs_ilock_attr_map_shared(dp);
> if (!xfs_inode_hasattr(dp)) {
> error = 0;
> } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> @@ -527,9 +527,7 @@ xfs_attr_list_int(
> } else {
> error = xfs_attr_node_list(context);
> }
> -
> - xfs_iunlock(dp, XFS_ILOCK_SHARED);
> -
> + xfs_iunlock(dp, lock_mode);
> return error;
> }
Looks good.
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 11/11] xfs: assert that we hold the ilock for extent map access
2013-12-06 20:30 [PATCH 00/11] extent list locking fixes V3 Christoph Hellwig
` (9 preceding siblings ...)
2013-12-06 20:30 ` [PATCH 10/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_list_int Christoph Hellwig
@ 2013-12-06 20:30 ` Christoph Hellwig
2013-12-08 22:40 ` Dave Chinner
10 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2013-12-06 20:30 UTC (permalink / raw)
To: xfs
[-- Attachment #1: iread_extents-assert --]
[-- Type: text/plain, Size: 2410 bytes --]
Make sure that xfs_bmapi_read has the ilock held in some way, and that
xfs_bmapi_write, xfs_bmapi_delay, xfs_bunmapi 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);
Index: xfs/fs/xfs/xfs_bmap.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap.c 2013-12-05 22:12:32.931838271 +0100
+++ xfs/fs/xfs/xfs_bmap.c 2013-12-05 22:35:59.175809412 +0100
@@ -5038,6 +5038,7 @@ xfs_bunmapi(
if (XFS_FORCED_SHUTDOWN(mp))
return XFS_ERROR(EIO);
+ ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
ASSERT(len > 0);
ASSERT(nexts >= 0);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 11/11] xfs: assert that we hold the ilock for extent map access
2013-12-06 20:30 ` [PATCH 11/11] xfs: assert that we hold the ilock for extent map access Christoph Hellwig
@ 2013-12-08 22:40 ` Dave Chinner
0 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2013-12-08 22:40 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Fri, Dec 06, 2013 at 12:30:17PM -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, xfs_bunmapi 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);
> Index: xfs/fs/xfs/xfs_bmap.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap.c 2013-12-05 22:12:32.931838271 +0100
> +++ xfs/fs/xfs/xfs_bmap.c 2013-12-05 22:35:59.175809412 +0100
> @@ -5038,6 +5038,7 @@ xfs_bunmapi(
> if (XFS_FORCED_SHUTDOWN(mp))
> return XFS_ERROR(EIO);
>
> + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> ASSERT(len > 0);
> ASSERT(nexts >= 0);
I can't think of anywhere else we need to add asserts for this, so
it seems fine to me.
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
^ permalink raw reply [flat|nested] 33+ messages in thread