Re: XFS/md/blkdev warning (was Re: Linux 2.6.26-rc2)

[Christoph Hellwig <hch@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

On Sat, May 17, 2008 at 02:17:37PM -0700, Linus Torvalds wrote:
> > [4294293.003500]  [<ffffffff8028106a>] __kmalloc+0x3e/0xe6
> > [4294293.003500]  [<ffffffff803067fc>] ? xfs_iflush_int+0x272/0x2fb
> > [4294293.003500]  [<ffffffff80320552>] kmem_alloc+0x6a/0xd1
> > [4294293.003500]  [<ffffffff80307a9c>] xfs_iflush_cluster+0x4b/0x33f
> > [4294293.003500]  [<ffffffff8030681e>] ? xfs_iflush_int+0x294/0x2fb
> > [4294293.003500]  [<ffffffff80307f4b>] xfs_iflush+0x1bb/0x29d
> > [4294293.003500]  [<ffffffff8031bc30>] xfs_inode_flush+0xb8/0xdd
> > [4294293.003500]  [<ffffffff80328b1f>] xfs_fs_write_inode+0x30/0x4c
> 
> And as a result, all the XFS stuff is then waiting for that lock which is 
> held by pdflush above:

Btw, just that function has a missing GFP_NOFS and a too large
allocation which were fixed by Dave Chinner but aren't in mainline
yet.  Can you check whether it still happens with the patch below?


[-- Attachment #2: xfs-icluster-add-nofs --]
[-- Type: text/plain, Size: 2165 bytes --]

On Thu, May 01, 2008 at 09:15:21AM -0400, Christoph Hellwig wrote:
> On Thu, May 01, 2008 at 10:26:11PM +1000, David Chinner wrote:
> > Index: 2.6.x-xfs-new/fs/xfs/xfs_inode.c
> > ===================================================================
> > --- 2.6.x-xfs-new.orig/fs/xfs/xfs_inode.c	2008-04-28 16:35:23.000000000 +1000
> > +++ 2.6.x-xfs-new/fs/xfs/xfs_inode.c	2008-05-01 20:04:55.151880341 +1000
> > @@ -2986,7 +2986,7 @@ xfs_iflush_cluster(
> >  	ASSERT(pag->pag_ici_init);
> >  
> >  	ilist_size = XFS_INODE_CLUSTER_SIZE(mp) * sizeof(xfs_inode_t *);
> > -	ilist = kmem_alloc(ilist_size, KM_MAYFAIL);
> > +	ilist = kmem_alloc(ilist_size, KM_NOFS);
> >  	if (!ilist)
> >  		return 0;
> 
> This should be KM_MAYFAIL | KM_NOFS, because KM_NOFS doesn't imply that
> the allocation may fail.

Yes, right you are - I only looked at the effect of __GFP_FS, not
what kmem_alloc does. i.e. kmem_flags_convert() doesn't do anything
with KM_MAYFAIL, forgetting that it's kmem_alloc() that uses it...

New patch below.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

---
Don't allow memory reclaim to wait on the filesystem in inode writeback

If we allow memory reclaim to wait on the pages under writeback in
inode cluster writeback we could deadlock because we are currently
holding the ILOCK on the initial writeback inode which is needed in
data I/O completion to change the file size or do unwritten extent
conversion before the pages are taken out of writeback state.

Signed-off-by: Dave Chinner <dgc@sgi.com>
---
 fs/xfs/xfs_inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: 2.6.x-xfs-new/fs/xfs/xfs_inode.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_inode.c	2008-04-28 16:35:23.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_inode.c	2008-05-02 08:03:30.071824780 +1000
@@ -2986,7 +2986,7 @@ xfs_iflush_cluster(
 	ASSERT(pag->pag_ici_init);
 
 	ilist_size = XFS_INODE_CLUSTER_SIZE(mp) * sizeof(xfs_inode_t *);
-	ilist = kmem_alloc(ilist_size, KM_MAYFAIL);
+	ilist = kmem_alloc(ilist_size, KM_MAYFAIL|KM_NOFS);
 	if (!ilist)
 		return 0;
 



[-- Attachment #3: xfs-fix-icluster-alloc-size --]
[-- Type: text/plain, Size: 1641 bytes --]

We only need to allocate space for the number of inodes in
the cluster when writing back inodes, not every byte in the
inode cluster. This reduces the amount of memory needing to
be allocated to 256 bytes instead of 64k.

Somebody pass me the brown paper bag, please.

Signed-off-by: Dave Chinner <dgc@sgi.com>
---
 fs/xfs/xfs_inode.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Index: 2.6.x-xfs-new/fs/xfs/xfs_inode.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_inode.c	2008-05-16 19:43:55.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_inode.c	2008-05-16 19:47:47.778141722 +1000
@@ -2913,6 +2913,7 @@ xfs_iflush_cluster(
 	xfs_mount_t		*mp = ip->i_mount;
 	xfs_perag_t		*pag = xfs_get_perag(mp, ip->i_ino);
 	unsigned long		first_index, mask;
+	unsigned long		inodes_per_cluster;
 	int			ilist_size;
 	xfs_inode_t		**ilist;
 	xfs_inode_t		*iq;
@@ -2924,7 +2925,8 @@ xfs_iflush_cluster(
 	ASSERT(pag->pagi_inodeok);
 	ASSERT(pag->pag_ici_init);
 
-	ilist_size = XFS_INODE_CLUSTER_SIZE(mp) * sizeof(xfs_inode_t *);
+	inodes_per_cluster = XFS_INODE_CLUSTER_SIZE(mp) >> mp->m_sb.sb_inodelog;
+	ilist_size = inodes_per_cluster * sizeof(xfs_inode_t *);
 	ilist = kmem_alloc(ilist_size, KM_MAYFAIL|KM_NOFS);
 	if (!ilist)
 		return 0;
@@ -2934,8 +2936,7 @@ xfs_iflush_cluster(
 	read_lock(&pag->pag_ici_lock);
 	/* really need a gang lookup range call here */
 	nr_found = radix_tree_gang_lookup(&pag->pag_ici_root, (void**)ilist,
-					first_index,
-					XFS_INODE_CLUSTER_SIZE(mp));
+					first_index, inodes_per_cluster);
 	if (nr_found == 0)
 		goto out_free;