[PATCH 0/9] stop taking the iolock in the reclaim path

[PATCH 0/9] stop taking the iolock in the reclaim path

[Christoph Hellwig]

Currently we take the iolock in the reclaim path in various places.
Taking it in the inode reclaim path means however that we can't take
it while doing memory allocations that recurse back into the filesystem,
and recently lockdep has been enhanced to find these cases and noticed
quite a few of these in XFS.

We had similar issues with the ilock, but we could get away with just
stopping to do filesystem-recursing allocation under the ilock as there
were just a few.  The iolock is however taken over larger critical sections
protection actual I/O and it's almost impossible to switch all these to
NOFS allocations.  Based on what the iolock is used for we don't actually
need it in the reclaim path, though - at this point the inode is dead
and no one has any other reference to it.  See the listing in the
first patch for a more detailed list of our current iolock holders and
why they can't contend with the reclaim path.

I would greatly appreciate some indepth-review of this to make sure I
haven't missed a big loophole..

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

[PATCH 1/9] xfs: avoid memory allocation under m_peraglock in growfs code

[Christoph Hellwig]

[-- Attachment #1: xfs-lockdep-growfs --]
[-- Type: text/plain, Size: 1590 bytes --]

Allocate the memory for the larger m_perag array before taking the
per-AG lock as the per-AG lock can be taken under the i_lock which
can be taken from reclaim context.

Reported by the new reclaim context tracing in lockdep.

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

Index: linux-2.6/fs/xfs/xfs_fsops.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_fsops.c	2009-07-10 13:05:24.798364926 +0200
+++ linux-2.6/fs/xfs/xfs_fsops.c	2009-07-10 13:16:00.827394975 +0200
@@ -167,17 +167,25 @@ xfs_growfs_data_private(
 	new = nb - mp->m_sb.sb_dblocks;
 	oagcount = mp->m_sb.sb_agcount;
 	if (nagcount > oagcount) {
+		void *new_perag, *old_perag;
+
 		xfs_filestream_flush(mp);
+
+		new_perag = kmem_zalloc(sizeof(xfs_perag_t) * nagcount,
+					KM_MAYFAIL);
+		if (!new_perag)
+			return XFS_ERROR(ENOMEM);
+
 		down_write(&mp->m_peraglock);
-		mp->m_perag = kmem_realloc(mp->m_perag,
-			sizeof(xfs_perag_t) * nagcount,
-			sizeof(xfs_perag_t) * oagcount,
-			KM_SLEEP);
-		memset(&mp->m_perag[oagcount], 0,
-			(nagcount - oagcount) * sizeof(xfs_perag_t));
+		memcpy(new_perag, mp->m_perag, sizeof(xfs_perag_t) * oagcount);
+		old_perag = mp->m_perag;
+		mp->m_perag = new_perag;
+
 		mp->m_flags |= XFS_MOUNT_32BITINODES;
 		nagimax = xfs_initialize_perag(mp, nagcount);
 		up_write(&mp->m_peraglock);
+
+		kmem_free(old_perag);
 	}
 	tp = xfs_trans_alloc(mp, XFS_TRANS_GROWFS);
 	tp->t_flags |= XFS_TRANS_RESERVE;

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

[PATCH 2/9] xfs: switch to NOFS allocation under i_lock in xfs_getbmap

[Christoph Hellwig]

[-- Attachment #1: xfs-lockdep-getbmap --]
[-- Type: text/plain, Size: 862 bytes --]

xfs_getbmap allocates memory with i_lock held, but i_lock is taken in
reclaim context so all allocations under it must avoid recursions into
the filesystem.

Reported by the new reclaim context tracing in lockdep.

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

Index: linux-2.6/fs/xfs/xfs_bmap.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_bmap.c	2009-07-10 13:05:24.808364312 +0200
+++ linux-2.6/fs/xfs/xfs_bmap.c	2009-07-10 13:16:00.830363579 +0200
@@ -6009,7 +6009,7 @@ xfs_getbmap(
 	 */
 	error = ENOMEM;
 	subnex = 16;
-	map = kmem_alloc(subnex * sizeof(*map), KM_MAYFAIL);
+	map = kmem_alloc(subnex * sizeof(*map), KM_MAYFAIL | KM_NOFS);
 	if (!map)
 		goto out_unlock_ilock;
 

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

[PATCH 3/9] xfs: switch to NOFS allocation under i_lock in xfs_da_state_alloc

[Christoph Hellwig]

[-- Attachment #1: xfs-lockdep-da_state_alloc --]
[-- Type: text/plain, Size: 877 bytes --]

xfs_da_state_alloc is always called with i_lock held, but i_lock is taken in
reclaim context so all allocations under it must avoid recursions into the
filesystem.

Reported by the new reclaim context tracing in lockdep.

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

Index: linux-2.6/fs/xfs/xfs_da_btree.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_da_btree.c	2009-07-10 13:05:24.812364541 +0200
+++ linux-2.6/fs/xfs/xfs_da_btree.c	2009-07-10 13:16:00.834365485 +0200
@@ -2201,7 +2201,7 @@ kmem_zone_t *xfs_dabuf_zone;		/* dabuf z
 xfs_da_state_t *
 xfs_da_state_alloc(void)
 {
-	return kmem_zone_zalloc(xfs_da_state_zone, KM_SLEEP);
+	return kmem_zone_zalloc(xfs_da_state_zone, KM_NOFS);
 }
 
 /*

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

[PATCH 4/9] xfs: switch to NOFS allocation under i_lock in xfs_da_buf_make

[Christoph Hellwig]

[-- Attachment #1: xfs-lockdep-xfs_da_buf_make --]
[-- Type: text/plain, Size: 974 bytes --]

i_lock is taken in the reclaim context so all allocations under it
must avoid recursions into the filesystem.

Reported by the new reclaim context tracing in lockdep.

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

Index: linux-2.6/fs/xfs/xfs_da_btree.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_da_btree.c	2009-07-10 13:05:24.812364541 +0200
+++ linux-2.6/fs/xfs/xfs_da_btree.c	2009-07-10 13:16:00.834365485 +0200
@@ -2261,9 +2261,9 @@ xfs_da_buf_make(int nbuf, xfs_buf_t **bp
 	int		off;
 
 	if (nbuf == 1)
-		dabuf = kmem_zone_alloc(xfs_dabuf_zone, KM_SLEEP);
+		dabuf = kmem_zone_alloc(xfs_dabuf_zone, KM_NOFS);
 	else
-		dabuf = kmem_alloc(XFS_DA_BUF_SIZE(nbuf), KM_SLEEP);
+		dabuf = kmem_alloc(XFS_DA_BUF_SIZE(nbuf), KM_NOFS);
 	dabuf->dirty = 0;
 #ifdef XFS_DABUF_DEBUG
 	dabuf->ra = ra;

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

[PATCH 5/9] xfs: switch to NOFS allocation under i_lock in xfs_dir_cilookup_result

[Christoph Hellwig]

[-- Attachment #1: xfs-lockdep-dir2 --]
[-- Type: text/plain, Size: 891 bytes --]

xfs_dir_cilookup_result is always called with i_lock held, but i_lock is taken
in reclaim context so all allocations under it must avoid recursions into the
filesystem.

Reported by the new reclaim context tracing in lockdep.

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

Index: linux-2.6/fs/xfs/xfs_dir2.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_dir2.c	2009-07-10 13:05:24.817364338 +0200
+++ linux-2.6/fs/xfs/xfs_dir2.c	2009-07-10 13:16:00.834365485 +0200
@@ -256,7 +256,7 @@ xfs_dir_cilookup_result(
 					!(args->op_flags & XFS_DA_OP_CILOOKUP))
 		return EEXIST;
 
-	args->value = kmem_alloc(len, KM_MAYFAIL);
+	args->value = kmem_alloc(len, KM_NOFS | KM_MAYFAIL);
 	if (!args->value)
 		return ENOMEM;
 

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

[PATCH 6/9] xfs: switch to NOFS allocation under i_lock in xfs_buf_associate_memory

[Christoph Hellwig]

[-- Attachment #1: xfs-lockdep-buf --]
[-- Type: text/plain, Size: 1312 bytes --]

xfs_buf_associate_memory is used for setting up the spare buffer for the
log wrap case in xlog_sync which can happen under i_lock when called from
xfs_fsync. The i_lock mutex is taken in reclaim context so all allocations
under it must avoid recursions into the filesystem.  There are a couple
more uses of xfs_buf_associate_memory in the log recovery code that are
also affected by this, but I'd rather keep the code simple than passing on
a gfp_mask argument.  Longer term we should just stop requiring the memoery
allocation in xlog_sync by some smaller rework of the buffer layer.

Reported by the new reclaim context tracing in lockdep.

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

Index: linux-2.6/fs/xfs/linux-2.6/xfs_buf.c
===================================================================
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_buf.c	2009-07-18 18:26:07.359681952 +0200
+++ linux-2.6/fs/xfs/linux-2.6/xfs_buf.c	2009-07-18 18:26:53.531658254 +0200
@@ -770,7 +770,7 @@ xfs_buf_associate_memory(
 	bp->b_pages = NULL;
 	bp->b_addr = mem;
 
-	rval = _xfs_buf_get_pages(bp, page_count, 0);
+	rval = _xfs_buf_get_pages(bp, page_count, XBF_DONT_BLOCK);
 	if (rval)
 		return rval;
 

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

[PATCH 7/9] xfs: switch to NOFS allocation under i_lock in xfs_attr_rmtval_set

[Christoph Hellwig]

[-- Attachment #1: xfs-lockdep-buf-2 --]
[-- Type: text/plain, Size: 1042 bytes --]

xfs_attr_rmtval_set is always called with i_lock held, and i_lock is taken
in reclaim context so all allocations under it must avoid recursions into
the filesystem.

Reported by the new reclaim context tracing in lockdep.

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

Index: linux-2.6/fs/xfs/xfs_attr.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_attr.c	2009-07-18 18:49:20.129533080 +0200
+++ linux-2.6/fs/xfs/xfs_attr.c	2009-07-18 18:50:01.713535041 +0200
@@ -2141,8 +2141,8 @@ xfs_attr_rmtval_set(xfs_da_args_t *args)
 		dblkno = XFS_FSB_TO_DADDR(mp, map.br_startblock),
 		blkcnt = XFS_FSB_TO_BB(mp, map.br_blockcount);
 
-		bp = xfs_buf_get_flags(mp->m_ddev_targp, dblkno,
-							blkcnt, XFS_BUF_LOCK);
+		bp = xfs_buf_get_flags(mp->m_ddev_targp, dblkno, blkcnt,
+				       XFS_BUF_LOCK | XBF_DONT_BLOCK);
 		ASSERT(bp);
 		ASSERT(!XFS_BUF_GETERROR(bp));
 

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

[PATCH 8/9] xfs: switch to NOFS allocation under i_lock in xfs_readlink_bmap

[Christoph Hellwig]

[-- Attachment #1: xfs-lockdep-buf-3 --]
[-- Type: text/plain, Size: 1057 bytes --]

xfs_readlink_bmap is called with i_lock held, but i_lock is taken in
reclaim context so all allocations under it must avoid recursions into
the filesystem.  

Reported by the new reclaim context tracing in lockdep.

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

Index: linux-2.6/fs/xfs/xfs_vnodeops.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_vnodeops.c	2009-07-18 20:29:59.041657600 +0200
+++ linux-2.6/fs/xfs/xfs_vnodeops.c	2009-07-18 20:31:22.964535322 +0200
@@ -547,7 +547,9 @@ xfs_readlink_bmap(
 		d = XFS_FSB_TO_DADDR(mp, mval[n].br_startblock);
 		byte_cnt = XFS_FSB_TO_B(mp, mval[n].br_blockcount);
 
-		bp = xfs_buf_read(mp->m_ddev_targp, d, BTOBB(byte_cnt), 0);
+		bp = xfs_buf_read_flags(mp->m_ddev_targp, d, BTOBB(byte_cnt),
+					XBF_LOCK | XBF_MAPPED |
+					XBF_DONT_BLOCK);
 		error = XFS_BUF_GETERROR(bp);
 		if (error) {
 			xfs_ioerror_alert("xfs_readlink",

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

[PATCH 9/9] xfs: switch to NOFS allocation under i_lock in xfs_attr_rmtval_get

[Christoph Hellwig]

[-- Attachment #1: xfs-lockdep-buf-4 --]
[-- Type: text/plain, Size: 975 bytes --]

xfs_attr_rmtval_get is always called with i_lock held, but i_lock is taken
in reclaim context so all allocations under it must avoid recursions into
the filesystem.  

Reported by the new reclaim context tracing in lockdep.


Index: linux-2.6/fs/xfs/xfs_attr.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_attr.c	2009-07-18 22:34:08.947532301 +0200
+++ linux-2.6/fs/xfs/xfs_attr.c	2009-07-18 22:35:44.786660003 +0200
@@ -2010,7 +2010,9 @@ xfs_attr_rmtval_get(xfs_da_args_t *args)
 			dblkno = XFS_FSB_TO_DADDR(mp, map[i].br_startblock);
 			blkcnt = XFS_FSB_TO_BB(mp, map[i].br_blockcount);
 			error = xfs_read_buf(mp, mp->m_ddev_targp, dblkno,
-					     blkcnt, XFS_BUF_LOCK, &bp);
+					     blkcnt,
+					     XFS_BUF_LOCK | XBF_DONT_BLOCK,
+					     &bp);
 			if (error)
 				return(error);
 

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

Re: [PATCH 0/9] stop taking the iolock in the reclaim path

[Christoph Hellwig]

Ah sorry, I resent the old lockdep patches,  Will send the real one
ASAP.

On Tue, Aug 25, 2009 at 02:21:34PM -0400, Christoph Hellwig wrote:
> Currently we take the iolock in the reclaim path in various places.
> Taking it in the inode reclaim path means however that we can't take
> it while doing memory allocations that recurse back into the filesystem,
> and recently lockdep has been enhanced to find these cases and noticed
> quite a few of these in XFS.
> 
> We had similar issues with the ilock, but we could get away with just
> stopping to do filesystem-recursing allocation under the ilock as there
> were just a few.  The iolock is however taken over larger critical sections
> protection actual I/O and it's almost impossible to switch all these to
> NOFS allocations.  Based on what the iolock is used for we don't actually
> need it in the reclaim path, though - at this point the inode is dead
> and no one has any other reference to it.  See the listing in the
> first patch for a more detailed list of our current iolock holders and
> why they can't contend with the reclaim path.
> 
> I would greatly appreciate some indepth-review of this to make sure I
> haven't missed a big loophole..
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---

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

[PATCH 0/9] fixes for memory allocator recursions into the filesystem

[Christoph Hellwig]

lockdep has recently start tracking if a lock is used in reclaim context
and if so warns if we do allocations without the NOFS flag inside it.

This patch series fixes the easy targers involving the i_lock by switching
various allocations to the NOFS variant, or in case of the growfs code
moving the allocation out of the lock.

We still have some more issues that involve the iolock, but I think we
can simply get rid of the iolock in the reclaim path, but I'll send that
our for a separate discussion.

Sage, this should fix the two reports you've sent a while ago.

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

[PATCH 9/9] xfs: switch to NOFS allocation under i_lock in xfs_attr_rmtval_get

[Christoph Hellwig]

[-- Attachment #1: xfs-lockdep-buf-4 --]
[-- Type: text/plain, Size: 975 bytes --]

xfs_attr_rmtval_get is always called with i_lock held, but i_lock is taken
in reclaim context so all allocations under it must avoid recursions into
the filesystem.  

Reported by the new reclaim context tracing in lockdep.


Index: linux-2.6/fs/xfs/xfs_attr.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_attr.c	2009-07-18 22:34:08.947532301 +0200
+++ linux-2.6/fs/xfs/xfs_attr.c	2009-07-18 22:35:44.786660003 +0200
@@ -2010,7 +2010,9 @@ xfs_attr_rmtval_get(xfs_da_args_t *args)
 			dblkno = XFS_FSB_TO_DADDR(mp, map[i].br_startblock);
 			blkcnt = XFS_FSB_TO_BB(mp, map[i].br_blockcount);
 			error = xfs_read_buf(mp, mp->m_ddev_targp, dblkno,
-					     blkcnt, XFS_BUF_LOCK, &bp);
+					     blkcnt,
+					     XFS_BUF_LOCK | XBF_DONT_BLOCK,
+					     &bp);
 			if (error)
 				return(error);
 

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