Message-ID: <20080205162703.GD7038@skywalker>
Date:	Tue, 5 Feb 2008 21:57:03 +0530
From:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
To:	Jan Kara <jack@...e.cz>
Cc:	Josef Bacik <jbacik@...hat.com>, Theodore Tso <tytso@....edu>,
	Andreas Dilger <adilger@....com>,
	Mingming Cao <cmm@...ibm.com>,
	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>
Subject: Re: jbd2_handle and i_data_sem circular locking dependency detected

On Tue, Feb 05, 2008 at 02:42:28PM +0100, Jan Kara wrote:
> On Tue 05-02-08 17:53:42, Aneesh Kumar K.V wrote:
> > 
> > How about the patch below. I did the below testing
> > a) migrate a file
> > b) run fs_inode fsstres fsx_linux.
> > 
> > The intention was to find out whether the new locking is breaking any of
> > the other expected hierarchy. It seems to fine. I didn't get any lockdep
> > warning.
>   I think there's a problem in the patch. I don't think you can call
> free_ind_block() while readers are accessing the file. That could make them
> think the file contains holes when they aren't there (or even worse read
> freed blocks or so). So you need to lock i_data_sem before this call (that
> means you have to move journal_extend() as well). Actually, I don't quite
> get why ext4_journal_extend() is called in that function. You can just
> count with the 1 credit in ext4_ext_migrate() when you call
> ext4_journal_extend() before calling ext4_ext_swap_inode_data(). Oh,
> probably because free_ind_block() could extend the transaction (which they
> don't do now as far as I can see).

ext4_journal_extend is called to extend the journal by one credit to
take care of writing the block containing inode. I moved the journal
extend before taking i_data_sem lock.

diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
index f97c993..dc6617a 100644
--- a/fs/ext4/migrate.c
+++ b/fs/ext4/migrate.c
@@ -302,10 +302,6 @@ static int ext4_ext_swap_inode_data(handle_t *handle, struct inode *inode,
 	struct ext4_inode_info *ei = EXT4_I(inode);
 	struct ext4_inode_info *tmp_ei = EXT4_I(tmp_inode);
 
-	retval = free_ind_block(handle, inode);
-	if (retval)
-		goto err_out;
-
 	/*
 	 * One credit accounted for writing the
 	 * i_data field of the original inode
@@ -318,6 +314,10 @@ static int ext4_ext_swap_inode_data(handle_t *handle, struct inode *inode,
 	}
 
 	down_write(&EXT4_I(inode)->i_data_sem);
+	retval = free_ind_block(handle, inode);
+	if (retval)
+		goto err_out;
+
 	/*
 	 * We have the extent map build with the tmp inode.
 	 * Now copy the i_data across

The above change also make sure we don't fail after we free the indirect
blocks.


>   BTW: That freeing code should really take into account that it can modify
> bitmaps in different block groups. It's even not that hard to do. Just
> before each ext4_free_blocks() in free_ind_block() you check whether you
> have still enough credits in the handle (use h_buffer_credits) and if not,
> extend it by some amount.


I have a FIXME at migrate.c:524 documenting exactly that. The
difficult question was by how much we should extent the journal. ? But
in reality we might have accumulated enough journal credits, I never
really ran across a case where we are running out of the journal credit.


>   Maybe you could do some test like writing a big file with some data and then
> while migration is running read it in a loop and compare that MD5SUM is the
> same all the time. Also run some memory-pressure during this test so that
> data blocks aren't cached for the whole time of the test... That should
> reasonably stress the migration code.

I have tested migrate by booting with mem= and doing parallel
read/write and migrate. I didn't do the MDSUM compare. Will do that this
time.


Thanks for all the help with review.
-aneesh
> 
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists