[PATCH 1/2] Fix possible UDF data corruption

[PATCH 1/2] Fix possible UDF data corruption

[Jan Kara]

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

  Hi Andrew,

  attached patch fixes possible data corruption in UDF - this bug was actually
introduced by one of my fixes :-( and should (if possible) go to Linus before
2.6.22 is out (that's why I'm diffing against Linus's tree and not the
latest changes in -mm tree)... Thanks.

								Honza

-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

[-- Attachment #2: udf-2.6.22-rc2-1-udf_data_corruption.diff --]
[-- Type: text/x-patch, Size: 1559 bytes --]

update_next_aext() could possibly rewrite values in elen and eloc, possibly
leading to data corruption when rewriting a file. Use temporary variables
instead. Also advance cur_epos as it can also point to an indirect extent
pointer.

Signed-off-by: Jan Kara <jack@suse.cz>

diff -rupX /home/jack/.kerndiffexclude linux-2.6.22-rc2/fs/udf/inode.c linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/inode.c
--- linux-2.6.22-rc2/fs/udf/inode.c	2007-05-24 18:00:05.000000000 +0200
+++ linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/inode.c	2007-05-24 18:16:36.000000000 +0200
@@ -460,8 +460,8 @@ static struct buffer_head * inode_getblk
 	kernel_long_ad laarr[EXTENT_MERGE_SIZE];
 	struct extent_position prev_epos, cur_epos, next_epos;
 	int count = 0, startnum = 0, endnum = 0;
-	uint32_t elen = 0;
-	kernel_lb_addr eloc;
+	uint32_t elen = 0, tmpelen;
+	kernel_lb_addr eloc, tmpeloc;
 	int c = 1;
 	loff_t lbcount = 0, b_off = 0;
 	uint32_t newblocknum, newblock;
@@ -520,8 +520,12 @@ static struct buffer_head * inode_getblk
 
 	b_off -= lbcount;
 	offset = b_off >> inode->i_sb->s_blocksize_bits;
-	/* Move into indirect extent if we are at a pointer to it */
-	udf_next_aext(inode, &prev_epos, &eloc, &elen, 0);
+	/*
+	 * Move prev_epos and cur_epos into indirect extent if we are at
+	 * the pointer to it
+	 */
+	udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, 0);
+	udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, 0);
 
 	/* if the extent is allocated and recorded, return the block
        if the extent is not a multiple of the blocksize, round up */

[PATCH 2/2] Fix possible leakage of blocks in UDF

[Jan Kara]

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

  Hello,

  attached is a patch that fixes possible leakage of free blocks / use of
free blocks in UDF (which spilled nice assertion failures I've added in my
first round of patches). More details in the changelog. Andrew, please apply.
Both changes have survived some time of fsx and fsstress testing so they
should be reasonably safe.

								Honza

[-- Attachment #2: udf-2.6.22-rc2-2-udf_block_leak.diff --]
[-- Type: text/x-patch, Size: 6795 bytes --]

It is wrong to call udf_discard_prealloc() from udf_clear_inode() as at that time
inode changes won't be written any more which can lead to leakage of blocks, use
of free blocks or improperly aligned extents. Also udf_discard_prealloc() does two
different things - it removes preallocated blocks and truncates the last extent to
exactly match i_size. We move the latter functionality to udf_truncate_tail_extent(),
call udf_discard_prealloc() when last reference to a file is dropped and call
udf_truncate_tail_extent() when inode is being removed from inode cach
(udf_drop_inode() call). We cannot call udf_truncate_tail_extent() earlier as
subsequent open+write would find the last block of the file mapped and happily write
to the end of it, although the last extent says it's shorter.

Signed-off-by: Jan Kara <jack@suse.cz>

diff -rupX /home/jack/.kerndiffexclude linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/inode.c linux-2.6.22-rc2-2-udf_block_leak/fs/udf/inode.c
--- linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/inode.c	2007-05-24 18:16:36.000000000 +0200
+++ linux-2.6.22-rc2-2-udf_block_leak/fs/udf/inode.c	2007-05-24 18:18:54.000000000 +0200
@@ -100,14 +100,18 @@ no_delete:
 	clear_inode(inode);
 }
 
-void udf_clear_inode(struct inode *inode)
+void udf_drop_inode(struct inode *inode)
 {
 	if (!(inode->i_sb->s_flags & MS_RDONLY)) {
 		lock_kernel();
-		udf_discard_prealloc(inode);
+		udf_truncate_tail_extent(inode);
 		unlock_kernel();
 	}
+	generic_drop_inode(inode);
+}
 
+void udf_clear_inode(struct inode *inode)
+{
 	kfree(UDF_I_DATA(inode));
 	UDF_I_DATA(inode) = NULL;
 }
diff -rupX /home/jack/.kerndiffexclude linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/super.c linux-2.6.22-rc2-2-udf_block_leak/fs/udf/super.c
--- linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/super.c	2007-05-24 18:00:05.000000000 +0200
+++ linux-2.6.22-rc2-2-udf_block_leak/fs/udf/super.c	2007-05-24 18:18:54.000000000 +0200
@@ -162,6 +162,7 @@ static const struct super_operations udf
 	.write_inode		= udf_write_inode,
 	.delete_inode		= udf_delete_inode,
 	.clear_inode		= udf_clear_inode,
+	.drop_inode		= udf_drop_inode,
 	.put_super		= udf_put_super,
 	.write_super		= udf_write_super,
 	.statfs			= udf_statfs,
diff -rupX /home/jack/.kerndiffexclude linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/truncate.c linux-2.6.22-rc2-2-udf_block_leak/fs/udf/truncate.c
--- linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/truncate.c	2007-05-24 18:00:05.000000000 +0200
+++ linux-2.6.22-rc2-2-udf_block_leak/fs/udf/truncate.c	2007-05-24 18:18:54.000000000 +0200
@@ -61,7 +61,11 @@ static void extent_trunc(struct inode * 
 	}
 }
 
-void udf_discard_prealloc(struct inode * inode)
+/*
+ * Truncate the last extent to match i_size. This function assumes
+ * that preallocation extent is already truncated.
+ */
+void udf_truncate_tail_extent(struct inode *inode)
 {
 	struct extent_position epos = { NULL, 0, {0, 0}};
 	kernel_lb_addr eloc;
@@ -71,7 +75,7 @@ void udf_discard_prealloc(struct inode *
 	int adsize;
 
 	if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_IN_ICB ||
-		inode->i_size == UDF_I_LENEXTENTS(inode))
+	    inode->i_size == UDF_I_LENEXTENTS(inode))
 		return;
 
 	if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_SHORT)
@@ -79,25 +83,63 @@ void udf_discard_prealloc(struct inode *
 	else if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_LONG)
 		adsize = sizeof(long_ad);
 	else
-		adsize = 0;
-
-	epos.block = UDF_I_LOCATION(inode);
+		BUG();
 
 	/* Find the last extent in the file */
 	while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1)
 	{
 		etype = netype;
 		lbcount += elen;
-		if (lbcount > inode->i_size && lbcount - elen < inode->i_size)
-		{
-			WARN_ON(lbcount - inode->i_size >= inode->i_sb->s_blocksize);
+		if (lbcount > inode->i_size) {
+			if (lbcount - inode->i_size >= inode->i_sb->s_blocksize)
+				printk(KERN_WARNING "udf_truncate_tail_extent():\
+ Too long extent after EOF in inode %u: i_size: %Ld lbcount: %Ld extent %u+%u\n",
+(unsigned)inode->i_ino, (long long)inode->i_size, (long long)lbcount,
+(unsigned)eloc.logicalBlockNum, (unsigned)elen);
 			nelen = elen - (lbcount - inode->i_size);
 			epos.offset -= adsize;
 			extent_trunc(inode, &epos, eloc, etype, elen, nelen);
 			epos.offset += adsize;
-			lbcount = inode->i_size;
+			if (udf_next_aext(inode, &epos, &eloc, &elen, 1) != -1)
+				printk(KERN_ERR "udf_truncate_tail_extent(): \
+Extent after EOF in inode %u.\n", (unsigned)inode->i_ino);
+			break;
 		}
 	}
+	/* This inode entry is in-memory only and thus we don't have to mark
+	 * the inode dirty */
+	UDF_I_LENEXTENTS(inode) = inode->i_size;
+	brelse(epos.bh);
+}
+
+void udf_discard_prealloc(struct inode * inode)
+{
+	struct extent_position epos = { NULL, 0, {0, 0}};
+	kernel_lb_addr eloc;
+	uint32_t elen;
+	uint64_t lbcount = 0;
+	int8_t etype = -1, netype;
+	int adsize;
+
+	if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_IN_ICB ||
+		inode->i_size == UDF_I_LENEXTENTS(inode))
+		return;
+
+	if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_SHORT)
+		adsize = sizeof(short_ad); 
+	else if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_LONG)
+		adsize = sizeof(long_ad);
+	else
+		adsize = 0;
+
+	epos.block = UDF_I_LOCATION(inode);
+
+	/* Find the last extent in the file */
+	while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1)
+	{
+		etype = netype;
+		lbcount += elen;
+	}
 	if (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) {
 		epos.offset -= adsize;
 		lbcount -= elen;
@@ -118,9 +160,9 @@ void udf_discard_prealloc(struct inode *
 			mark_buffer_dirty_inode(epos.bh, inode);
 		}
 	}
+	/* This inode entry is in-memory only and thus we don't have to mark
+	 * the inode dirty */
 	UDF_I_LENEXTENTS(inode) = lbcount;
-
-	WARN_ON(lbcount != inode->i_size);
 	brelse(epos.bh);
 }
 
diff -rupX /home/jack/.kerndiffexclude linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/udfdecl.h linux-2.6.22-rc2-2-udf_block_leak/fs/udf/udfdecl.h
--- linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/udfdecl.h	2007-05-24 18:00:05.000000000 +0200
+++ linux-2.6.22-rc2-2-udf_block_leak/fs/udf/udfdecl.h	2007-05-24 18:18:54.000000000 +0200
@@ -103,6 +103,7 @@ extern struct buffer_head * udf_bread(st
 extern void udf_truncate(struct inode *);
 extern void udf_read_inode(struct inode *);
 extern void udf_delete_inode(struct inode *);
+extern void udf_drop_inode(struct inode *);
 extern void udf_clear_inode(struct inode *);
 extern int udf_write_inode(struct inode *, int);
 extern long udf_block_map(struct inode *, sector_t);
@@ -146,6 +147,7 @@ extern void udf_free_inode(struct inode 
 extern struct inode * udf_new_inode (struct inode *, int, int *);
 
 /* truncate.c */
+extern void udf_truncate_tail_extent(struct inode *);
 extern void udf_discard_prealloc(struct inode *);
 extern void udf_truncate_extents(struct inode *);
 

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Jan Kara]

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

  Hello,

On Thu 24-05-07 19:05:54, Jan Kara wrote:
>   Hello,
> 
>   attached is a patch that fixes possible leakage of free blocks / use of
> free blocks in UDF (which spilled nice assertion failures I've added in my
> first round of patches). More details in the changelog. Andrew, please apply.
> Both changes have survived some time of fsx and fsstress testing so they
> should be reasonably safe.
  Sorry for replying to myself but this patch had a minor problem of
printing some bogus warnings when directories were deleted (I wonder why
fsstress didn't find it). Attached is a new version of the patch without
this problem.

									Honza

[-- Attachment #2: udf-2.6.22-rc2-2-udf_block_leak.diff --]
[-- Type: text/x-patch, Size: 6945 bytes --]

It is wrong to call udf_discard_prealloc() from udf_clear_inode() as at that time
inode changes won't be written any more which can lead to leakage of blocks, use
of free blocks or improperly aligned extents. Also udf_discard_prealloc() does two
different things - it removes preallocated blocks and truncates the last extent to
exactly match i_size. We move the latter functionality to udf_truncate_tail_extent(),
call udf_discard_prealloc() when last reference to a file is dropped and call
udf_truncate_tail_extent() when inode is being removed from inode cach
(udf_drop_inode() call). We cannot call udf_truncate_tail_extent() earlier as
subsequent open+write would find the last block of the file mapped and happily write
to the end of it, although the last extent says it's shorter.

Signed-off-by: Jan Kara <jack@suse.cz>

diff -rupX /home/jack/.kerndiffexclude linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/inode.c linux-2.6.22-rc2-2-udf_block_leak/fs/udf/inode.c
--- linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/inode.c	2007-05-24 18:16:36.000000000 +0200
+++ linux-2.6.22-rc2-2-udf_block_leak/fs/udf/inode.c	2007-05-24 21:13:59.000000000 +0200
@@ -100,14 +100,20 @@ no_delete:
 	clear_inode(inode);
 }
 
-void udf_clear_inode(struct inode *inode)
+void udf_drop_inode(struct inode *inode)
 {
 	if (!(inode->i_sb->s_flags & MS_RDONLY)) {
 		lock_kernel();
+		/* Discard preallocation for directories, symlinks, etc. */
 		udf_discard_prealloc(inode);
+		udf_truncate_tail_extent(inode);
 		unlock_kernel();
 	}
+	generic_drop_inode(inode);
+}
 
+void udf_clear_inode(struct inode *inode)
+{
 	kfree(UDF_I_DATA(inode));
 	UDF_I_DATA(inode) = NULL;
 }
diff -rupX /home/jack/.kerndiffexclude linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/super.c linux-2.6.22-rc2-2-udf_block_leak/fs/udf/super.c
--- linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/super.c	2007-05-24 18:00:05.000000000 +0200
+++ linux-2.6.22-rc2-2-udf_block_leak/fs/udf/super.c	2007-05-24 18:18:54.000000000 +0200
@@ -162,6 +162,7 @@ static const struct super_operations udf
 	.write_inode		= udf_write_inode,
 	.delete_inode		= udf_delete_inode,
 	.clear_inode		= udf_clear_inode,
+	.drop_inode		= udf_drop_inode,
 	.put_super		= udf_put_super,
 	.write_super		= udf_write_super,
 	.statfs			= udf_statfs,
diff -rupX /home/jack/.kerndiffexclude linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/truncate.c linux-2.6.22-rc2-2-udf_block_leak/fs/udf/truncate.c
--- linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/truncate.c	2007-05-24 18:00:05.000000000 +0200
+++ linux-2.6.22-rc2-2-udf_block_leak/fs/udf/truncate.c	2007-05-24 21:31:38.000000000 +0200
@@ -61,7 +61,11 @@ static void extent_trunc(struct inode * 
 	}
 }
 
-void udf_discard_prealloc(struct inode * inode)
+/*
+ * Truncate the last extent to match i_size. This function assumes
+ * that preallocation extent is already truncated.
+ */
+void udf_truncate_tail_extent(struct inode *inode)
 {
 	struct extent_position epos = { NULL, 0, {0, 0}};
 	kernel_lb_addr eloc;
@@ -71,7 +75,10 @@ void udf_discard_prealloc(struct inode *
 	int adsize;
 
 	if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_IN_ICB ||
-		inode->i_size == UDF_I_LENEXTENTS(inode))
+	    inode->i_size == UDF_I_LENEXTENTS(inode))
+		return;
+	/* Are we going to delete the file anyway? */
+	if (inode->i_nlink == 0)
 		return;
 
 	if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_SHORT)
@@ -79,25 +86,63 @@ void udf_discard_prealloc(struct inode *
 	else if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_LONG)
 		adsize = sizeof(long_ad);
 	else
-		adsize = 0;
-
-	epos.block = UDF_I_LOCATION(inode);
+		BUG();
 
 	/* Find the last extent in the file */
 	while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1)
 	{
 		etype = netype;
 		lbcount += elen;
-		if (lbcount > inode->i_size && lbcount - elen < inode->i_size)
-		{
-			WARN_ON(lbcount - inode->i_size >= inode->i_sb->s_blocksize);
+		if (lbcount > inode->i_size) {
+			if (lbcount - inode->i_size >= inode->i_sb->s_blocksize)
+				printk(KERN_WARNING "udf_truncate_tail_extent():\
+ Too long extent after EOF in inode %u: i_size: %Ld lbcount: %Ld extent %u+%u\n",
+(unsigned)inode->i_ino, (long long)inode->i_size, (long long)lbcount,
+(unsigned)eloc.logicalBlockNum, (unsigned)elen);
 			nelen = elen - (lbcount - inode->i_size);
 			epos.offset -= adsize;
 			extent_trunc(inode, &epos, eloc, etype, elen, nelen);
 			epos.offset += adsize;
-			lbcount = inode->i_size;
+			if (udf_next_aext(inode, &epos, &eloc, &elen, 1) != -1)
+				printk(KERN_ERR "udf_truncate_tail_extent(): \
+Extent after EOF in inode %u.\n", (unsigned)inode->i_ino);
+			break;
 		}
 	}
+	/* This inode entry is in-memory only and thus we don't have to mark
+	 * the inode dirty */
+	UDF_I_LENEXTENTS(inode) = inode->i_size;
+	brelse(epos.bh);
+}
+
+void udf_discard_prealloc(struct inode * inode)
+{
+	struct extent_position epos = { NULL, 0, {0, 0}};
+	kernel_lb_addr eloc;
+	uint32_t elen;
+	uint64_t lbcount = 0;
+	int8_t etype = -1, netype;
+	int adsize;
+
+	if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_IN_ICB ||
+		inode->i_size == UDF_I_LENEXTENTS(inode))
+		return;
+
+	if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_SHORT)
+		adsize = sizeof(short_ad); 
+	else if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_LONG)
+		adsize = sizeof(long_ad);
+	else
+		adsize = 0;
+
+	epos.block = UDF_I_LOCATION(inode);
+
+	/* Find the last extent in the file */
+	while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1)
+	{
+		etype = netype;
+		lbcount += elen;
+	}
 	if (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) {
 		epos.offset -= adsize;
 		lbcount -= elen;
@@ -118,9 +163,9 @@ void udf_discard_prealloc(struct inode *
 			mark_buffer_dirty_inode(epos.bh, inode);
 		}
 	}
+	/* This inode entry is in-memory only and thus we don't have to mark
+	 * the inode dirty */
 	UDF_I_LENEXTENTS(inode) = lbcount;
-
-	WARN_ON(lbcount != inode->i_size);
 	brelse(epos.bh);
 }
 
diff -rupX /home/jack/.kerndiffexclude linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/udfdecl.h linux-2.6.22-rc2-2-udf_block_leak/fs/udf/udfdecl.h
--- linux-2.6.22-rc2-1-udf_data_corruption/fs/udf/udfdecl.h	2007-05-24 18:00:05.000000000 +0200
+++ linux-2.6.22-rc2-2-udf_block_leak/fs/udf/udfdecl.h	2007-05-24 18:18:54.000000000 +0200
@@ -103,6 +103,7 @@ extern struct buffer_head * udf_bread(st
 extern void udf_truncate(struct inode *);
 extern void udf_read_inode(struct inode *);
 extern void udf_delete_inode(struct inode *);
+extern void udf_drop_inode(struct inode *);
 extern void udf_clear_inode(struct inode *);
 extern int udf_write_inode(struct inode *, int);
 extern long udf_block_map(struct inode *, sector_t);
@@ -146,6 +147,7 @@ extern void udf_free_inode(struct inode 
 extern struct inode * udf_new_inode (struct inode *, int, int *);
 
 /* truncate.c */
+extern void udf_truncate_tail_extent(struct inode *);
 extern void udf_discard_prealloc(struct inode *);
 extern void udf_truncate_extents(struct inode *);
 

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Eric Sandeen]

Jan Kara wrote:
>   Hello,
> 
> On Thu 24-05-07 19:05:54, Jan Kara wrote:
>>   Hello,
>>
>>   attached is a patch that fixes possible leakage of free blocks / use of
>> free blocks in UDF (which spilled nice assertion failures I've added in my
>> first round of patches). More details in the changelog. Andrew, please apply.
>> Both changes have survived some time of fsx and fsstress testing so they
>> should be reasonably safe.
>   Sorry for replying to myself but this patch had a minor problem of
> printing some bogus warnings when directories were deleted (I wonder why
> fsstress didn't find it). Attached is a new version of the patch without
> this problem.

Jan, something seems busted here.  I'm getting lockups when testing udf
on a single cpu with this last patch in place...

I think it's the BKL stumbling on itself.

for example...

static int udf_symlink(struct inode * dir, struct dentry * dentry, const
char * symname)
{
...
        lock_kernel();
...
out:
        unlock_kernel();
        return err;

out_no_entry:
        inode_dec_link_count(inode);
        iput(inode);
        goto out;
}

but iput goes
iput->iput_final->drop_inode->udf_drop_inode->lock_kernel() again

looking for the right way around it but figured I'd ping you early :)

-Eric

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Eric Sandeen]

Eric Sandeen wrote:

> Jan, something seems busted here.  I'm getting lockups when testing udf
> on a single cpu with this last patch in place...
> 
> I think it's the BKL stumbling on itself.

To demonstrate, try this:

# BIGFILENAME=`seq -s '' 1 1000`
# ln -s $BIGFILENAME foo

instant deadlock :(

-Eric

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Eric Sandeen - Wed, May 30, 2007 at 05:22:23PM -0500]
| Eric Sandeen wrote:
| 
| > Jan, something seems busted here.  I'm getting lockups when testing udf
| > on a single cpu with this last patch in place...
| > 
| > I think it's the BKL stumbling on itself.
| 
| To demonstrate, try this:
| 
| # BIGFILENAME=`seq -s '' 1 1000`
| # ln -s $BIGFILENAME foo
| 
| instant deadlock :(
| 
| -Eric
| 

Hi, thanks for error report. I'm checking this tommorow I guess
(too busy now)

		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Eric Sandeen - Wed, May 30, 2007 at 05:22:23PM -0500]
| Eric Sandeen wrote:
| 
| > Jan, something seems busted here.  I'm getting lockups when testing udf
| > on a single cpu with this last patch in place...
| > 
| > I think it's the BKL stumbling on itself.
| 
| To demonstrate, try this:
| 
| # BIGFILENAME=`seq -s '' 1 1000`
| # ln -s $BIGFILENAME foo
| 
| instant deadlock :(
| 
| -Eric
| 

Eric, could you please try the following:

1) declare the spinlock in the top of inode.c as

	DEFINE_SPINLOCK(udf_drop_lock);

2) replace in udf_drop_inode()

	kernel_lock -> spin_lock(&udf_drop_lock);
	kernel_unlock -> spin_unlock(&udf_drop_lock);

I'm not sure if it help but you may try ;)

		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Eric Sandeen]

Cyrill Gorcunov wrote:

> Eric, could you please try the following:
> 
> 1) declare the spinlock in the top of inode.c as
> 
> 	DEFINE_SPINLOCK(udf_drop_lock);
> 
> 2) replace in udf_drop_inode()
> 
> 	kernel_lock -> spin_lock(&udf_drop_lock);
> 	kernel_unlock -> spin_unlock(&udf_drop_lock);
> 
> I'm not sure if it help but you may try ;)
> 
> 		Cyrill
> 

I'm sure it'll avoid the deadlock but....

Any sense of what the BKL is actually trying to protect in this case?

Is it really only trying to prevent concurrent prealloc-discarders, or 
is there more?

-Eric

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Eric Sandeen - Thu, May 31, 2007 at 12:46:15PM -0500]
| Cyrill Gorcunov wrote:
| 
| >Eric, could you please try the following:
| >
| >1) declare the spinlock in the top of inode.c as
| >
| >	DEFINE_SPINLOCK(udf_drop_lock);
| >
| >2) replace in udf_drop_inode()
| >
| >	kernel_lock -> spin_lock(&udf_drop_lock);
| >	kernel_unlock -> spin_unlock(&udf_drop_lock);
| >
| >I'm not sure if it help but you may try ;)
| >
| >		Cyrill
| >
| 
| I'm sure it'll avoid the deadlock but....
| 
| Any sense of what the BKL is actually trying to protect in this case?
| 
| Is it really only trying to prevent concurrent prealloc-discarders, or 
| is there more?
| 
| -Eric
| 

Hi Eric,
it seems BKL only trying to protect from concurrent discard_prealloc.
Moreover, a lot of UDF code does call iput with BKL held, so the only
solution I see is to add spinlocks to udf_drop_inode... I'm making patch
soon. Any comments?

		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Andrew Morton]

On Fri, 1 Jun 2007 20:49:26 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Eric Sandeen - Thu, May 31, 2007 at 12:46:15PM -0500]
> | Cyrill Gorcunov wrote:
> | 
> | >Eric, could you please try the following:
> | >
> | >1) declare the spinlock in the top of inode.c as
> | >
> | >	DEFINE_SPINLOCK(udf_drop_lock);
> | >
> | >2) replace in udf_drop_inode()
> | >
> | >	kernel_lock -> spin_lock(&udf_drop_lock);
> | >	kernel_unlock -> spin_unlock(&udf_drop_lock);
> | >
> | >I'm not sure if it help but you may try ;)
> | >
> | >		Cyrill
> | >
> | 
> | I'm sure it'll avoid the deadlock but....
> | 
> | Any sense of what the BKL is actually trying to protect in this case?
> | 
> | Is it really only trying to prevent concurrent prealloc-discarders, or 
> | is there more?
> | 
> | -Eric
> | 
> 
> Hi Eric,
> it seems BKL only trying to protect from concurrent discard_prealloc.
> Moreover, a lot of UDF code does call iput with BKL held, so the only
> solution I see is to add spinlocks to udf_drop_inode... I'm making patch
> soon. Any comments?
> 

Recursive lock_kernel() is OK.

spin_lock() insode lock_kernel() is OK.

lock_kernel() inside spin_lock() is not OK, but if this was happening you'd
only rarely hit a deadlock and I think this locks up every time.

We don't know what's causing this hang, do we?

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Andrew Morton - Fri, Jun 01, 2007 at 10:04:25AM -0700]
| On Fri, 1 Jun 2007 20:49:26 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > [Eric Sandeen - Thu, May 31, 2007 at 12:46:15PM -0500]
| > | Cyrill Gorcunov wrote:
| > | 
| > | >Eric, could you please try the following:
| > | >
| > | >1) declare the spinlock in the top of inode.c as
| > | >
| > | >	DEFINE_SPINLOCK(udf_drop_lock);
| > | >
| > | >2) replace in udf_drop_inode()
| > | >
| > | >	kernel_lock -> spin_lock(&udf_drop_lock);
| > | >	kernel_unlock -> spin_unlock(&udf_drop_lock);
| > | >
| > | >I'm not sure if it help but you may try ;)
| > | >
| > | >		Cyrill
| > | >
| > | 
| > | I'm sure it'll avoid the deadlock but....
| > | 
| > | Any sense of what the BKL is actually trying to protect in this case?
| > | 
| > | Is it really only trying to prevent concurrent prealloc-discarders, or 
| > | is there more?
| > | 
| > | -Eric
| > | 
| > 
| > Hi Eric,
| > it seems BKL only trying to protect from concurrent discard_prealloc.
| > Moreover, a lot of UDF code does call iput with BKL held, so the only
| > solution I see is to add spinlocks to udf_drop_inode... I'm making patch
| > soon. Any comments?
| > 
| 
| Recursive lock_kernel() is OK.
| 
| spin_lock() insode lock_kernel() is OK.
| 
| lock_kernel() inside spin_lock() is not OK, but if this was happening you'd
| only rarely hit a deadlock and I think this locks up every time.
| 
| We don't know what's causing this hang, do we?
| 

Actually I've read in Robert Love book that recursive kernel_lock is
supposed but as Eric wrote "he's sure spin_lock will avoid deadlock"...
So I'm investigating this (unfortunelly can't test it myself - have
UDF compiled into kernel :(.

		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Eric Sandeen]

Andrew Morton wrote:

> Recursive lock_kernel() is OK.

Oh, it is?  Clearly I am not well versed in the BKL... that's probably a 
good thing.... :)

Ok, let me look into it further.  I changed lock_kernel to 
udf_lock_kernel to complain & backtrace if we re-lock, and it always 
immediately hung after that; I assumed that was it.  I'll investigate 
further.

-Eric

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Eric Sandeen - Fri, Jun 01, 2007 at 12:17:53PM -0500]
| Andrew Morton wrote:
| 
| >Recursive lock_kernel() is OK.
| 
| Oh, it is?  Clearly I am not well versed in the BKL... that's probably a 
| good thing.... :)
| 
| Ok, let me look into it further.  I changed lock_kernel to 
| udf_lock_kernel to complain & backtrace if we re-lock, and it always 
| immediately hung after that; I assumed that was it.  I'll investigate 
| further.
| 
| -Eric
| 

Btw, Andrew is there any way to force kernel to use special UDF module
instead of compiled-in one? (Sorry for stupid question ;)

		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Eric Sandeen]

Cyrill Gorcunov wrote:
> [Eric Sandeen - Fri, Jun 01, 2007 at 12:17:53PM -0500]
> | Andrew Morton wrote:
> | 
> | >Recursive lock_kernel() is OK.
> | 
> | Oh, it is?  Clearly I am not well versed in the BKL... that's probably a 
> | good thing.... :)
> | 
> | Ok, let me look into it further.  I changed lock_kernel to 
> | udf_lock_kernel to complain & backtrace if we re-lock, and it always 
> | immediately hung after that; I assumed that was it.  I'll investigate 
> | further.
> | 
> | -Eric
> | 
> 
> Btw, Andrew is there any way to force kernel to use special UDF module
> instead of compiled-in one? (Sorry for stupid question ;)

Not if it's  already built in (at least not with more hackery than it's 
worth...) - just rebuild your kernel w/ udf as a module.

BTW my testcase before was bogus, that's not what's causing the lockup. 
  I'll keep investigating now that I know what *not* to look for. ;-)

-Eric

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Eric Sandeen - Fri, Jun 01, 2007 at 12:51:34PM -0500]
| Cyrill Gorcunov wrote:
| >[Eric Sandeen - Fri, Jun 01, 2007 at 12:17:53PM -0500]
| >| Andrew Morton wrote:
| >| 
| >| >Recursive lock_kernel() is OK.
| >| 
| >| Oh, it is?  Clearly I am not well versed in the BKL... that's probably a 
| >| good thing.... :)
| >| 
| >| Ok, let me look into it further.  I changed lock_kernel to 
| >| udf_lock_kernel to complain & backtrace if we re-lock, and it always 
| >| immediately hung after that; I assumed that was it.  I'll investigate 
| >| further.
| >| 
| >| -Eric
| >| 
| >
| >Btw, Andrew is there any way to force kernel to use special UDF module
| >instead of compiled-in one? (Sorry for stupid question ;)
| 
| Not if it's  already built in (at least not with more hackery than it's 
| worth...) - just rebuild your kernel w/ udf as a module.
| 
| BTW my testcase before was bogus, that's not what's causing the lockup. 
|  I'll keep investigating now that I know what *not* to look for. ;-)
| 
| -Eric
| 

Thanks for answer

		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

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

[Eric Sandeen - Fri, Jun 01, 2007 at 12:51:34PM -0500]
| Cyrill Gorcunov wrote:
| >[Eric Sandeen - Fri, Jun 01, 2007 at 12:17:53PM -0500]
| >| Andrew Morton wrote:
| >| 
| >| >Recursive lock_kernel() is OK.
| >| 
| >| Oh, it is?  Clearly I am not well versed in the BKL... that's probably a 
| >| good thing.... :)
| >| 
| >| Ok, let me look into it further.  I changed lock_kernel to 
| >| udf_lock_kernel to complain & backtrace if we re-lock, and it always 
| >| immediately hung after that; I assumed that was it.  I'll investigate 
| >| further.
| >| 
| >| -Eric
| >| 
| >
| >Btw, Andrew is there any way to force kernel to use special UDF module
| >instead of compiled-in one? (Sorry for stupid question ;)
| 
| Not if it's  already built in (at least not with more hackery than it's 
| worth...) - just rebuild your kernel w/ udf as a module.
| 
| BTW my testcase before was bogus, that's not what's causing the lockup. 
|  I'll keep investigating now that I know what *not* to look for. ;-)
| 
| -Eric
| 

Eric, could you try this

		Cyrill


[-- Attachment #2: att-01.diff --]
[-- Type: text/plain, Size: 506 bytes --]

diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index 51fe307..833c1b6 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -983,6 +983,8 @@ static int udf_symlink(struct inode * dir, struct dentry * dentry, const char *
 		block = udf_get_pblock(inode->i_sb, block,
 			UDF_I_LOCATION(inode).partitionReferenceNum, 0);
 		epos.bh = udf_tread(inode->i_sb, block);
+		if (!epos.bh)
+			BUG();
 		lock_buffer(epos.bh);
 		memset(epos.bh->b_data, 0x00, inode->i_sb->s_blocksize);
 		set_buffer_uptodate(epos.bh);

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Jan Kara]

On Wed 30-05-07 16:46:28, Eric Sandeen wrote:
> Jan Kara wrote:
> >   Hello,
> > 
> > On Thu 24-05-07 19:05:54, Jan Kara wrote:
> >>   Hello,
> >>
> >>   attached is a patch that fixes possible leakage of free blocks / use of
> >> free blocks in UDF (which spilled nice assertion failures I've added in my
> >> first round of patches). More details in the changelog. Andrew, please apply.
> >> Both changes have survived some time of fsx and fsstress testing so they
> >> should be reasonably safe.
> >   Sorry for replying to myself but this patch had a minor problem of
> > printing some bogus warnings when directories were deleted (I wonder why
> > fsstress didn't find it). Attached is a new version of the patch without
> > this problem.
> 
> Jan, something seems busted here.  I'm getting lockups when testing udf
> on a single cpu with this last patch in place...
  Hmm, strange, I was also testing on UP and without problems. And I didn't
change any locking...

> I think it's the BKL stumbling on itself.
> 
> for example...
> 
> static int udf_symlink(struct inode * dir, struct dentry * dentry, const
> char * symname)
> {
> ...
>         lock_kernel();
> ...
> out:
>         unlock_kernel();
>         return err;
> 
> out_no_entry:
>         inode_dec_link_count(inode);
>         iput(inode);
>         goto out;
> }
> 
> but iput goes
> iput->iput_final->drop_inode->udf_drop_inode->lock_kernel() again
  As Andrew already wrote, BKL is free to recurse...

> looking for the right way around it but figured I'd ping you early :)
  Thanks for info - I'm now mostly out of email for a few days but I'll
have a look at it as soon as I return.

								Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Eric Sandeen]

Jan Kara wrote:

>> but iput goes
>> iput->iput_final->drop_inode->udf_drop_inode->lock_kernel() again
>   As Andrew already wrote, BKL is free to recurse...
> 
>> looking for the right way around it but figured I'd ping you early :)
>   Thanks for info - I'm now mostly out of email for a few days but I'll
> have a look at it as soon as I return.

Ok.  I was offbase in my first analysis (sorry!) but something -is- 
definitely locking up when I run fsstress, and w/o the patch mentioned 
it's ok.

I'm trying to manage to get it sorted but i'm out a couple of days now, too.

-Eric

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Eric Sandeen]

Jan Kara wrote:
>   Thanks for info - I'm now mostly out of email for a few days but I'll
> have a look at it as soon as I return.
> 
> 								Honza

Here we go:

[1]kdb> btp 3263
Stack traceback for pid 3263
0xffff81006f1b8100     3263     3247  1    0   R  0xffff81006f1b83d0 
lt-fsstress
rsp                rip                Function (args)
0xffff81006f335c48 0xffffffff8044b989 _spin_lock+0x7
0xffff81006f335c70 0xffffffff802a914b __mark_inode_dirty+0xe0
0xffff81006f335ca0 0xffffffff882d47d0 [udf]udf_write_aext+0x101
0xffff81006f335cd0 0xffffffff882dd95a [udf]extent_trunc+0xd6
0xffff81006f335d20 0xffffffff882dda81 [udf]udf_truncate_tail_extent+0xda
0xffff81006f335da0 0xffffffff882d7c26 [udf]udf_drop_inode+0x26
0xffff81006f335db0 0xffffffff8029f816 iput+0x74
0xffff81006f335dc0 0xffffffff8029d6a8 dentry_iput+0x8f
0xffff81006f335de0 0xffffffff8029d748 d_kill+0x21
0xffff81006f335e00 0xffffffff8029e450 prune_one_dentry+0x3a
0xffff81006f335e20 0xffffffff8029e5e8 prune_dcache+0xe3
0xffff81006f335e50 0xffffffff8029e6bf shrink_dcache_parent+0x21
0xffff81006f335e80 0xffffffff80294a27 dentry_unhash+0x1d
0xffff81006f335e90 0xffffffff80295d43 vfs_rmdir+0x88
0xffff81006f335eb0 0xffffffff80297d5b do_rmdir+0x9c
0xffff81006f335f40 0xffffffff8020ce23 syscall_trace_enter+0x8d
0xffff81006f335f70 0xffffffff80297dd4 sys_rmdir+0x11
0xffff81006f335f80 0xffffffff80209d5c tracesys+0xdc

going for the inode_lock twice?

-Eric

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Andrew Morton]

On Fri, 01 Jun 2007 17:37:49 -0500
Eric Sandeen <sandeen@sandeen.net> wrote:

> Jan Kara wrote:
> >   Thanks for info - I'm now mostly out of email for a few days but I'll
> > have a look at it as soon as I return.
> > 
> > 								Honza
> 
> Here we go:
> 
> [1]kdb> btp 3263
> Stack traceback for pid 3263
> 0xffff81006f1b8100     3263     3247  1    0   R  0xffff81006f1b83d0 
> lt-fsstress
> rsp                rip                Function (args)
> 0xffff81006f335c48 0xffffffff8044b989 _spin_lock+0x7
> 0xffff81006f335c70 0xffffffff802a914b __mark_inode_dirty+0xe0
> 0xffff81006f335ca0 0xffffffff882d47d0 [udf]udf_write_aext+0x101
> 0xffff81006f335cd0 0xffffffff882dd95a [udf]extent_trunc+0xd6
> 0xffff81006f335d20 0xffffffff882dda81 [udf]udf_truncate_tail_extent+0xda
> 0xffff81006f335da0 0xffffffff882d7c26 [udf]udf_drop_inode+0x26
> 0xffff81006f335db0 0xffffffff8029f816 iput+0x74
> 0xffff81006f335dc0 0xffffffff8029d6a8 dentry_iput+0x8f
> 0xffff81006f335de0 0xffffffff8029d748 d_kill+0x21
> 0xffff81006f335e00 0xffffffff8029e450 prune_one_dentry+0x3a
> 0xffff81006f335e20 0xffffffff8029e5e8 prune_dcache+0xe3
> 0xffff81006f335e50 0xffffffff8029e6bf shrink_dcache_parent+0x21
> 0xffff81006f335e80 0xffffffff80294a27 dentry_unhash+0x1d
> 0xffff81006f335e90 0xffffffff80295d43 vfs_rmdir+0x88
> 0xffff81006f335eb0 0xffffffff80297d5b do_rmdir+0x9c
> 0xffff81006f335f40 0xffffffff8020ce23 syscall_trace_enter+0x8d
> 0xffff81006f335f70 0xffffffff80297dd4 sys_rmdir+0x11
> 0xffff81006f335f80 0xffffffff80209d5c tracesys+0xdc
> 
> going for the inode_lock twice?
> 

lockdep should catch that.

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Eric Sandeen]

Andrew Morton wrote:
> On Fri, 01 Jun 2007 17:37:49 -0500
> Eric Sandeen <sandeen@sandeen.net> wrote:

>> going for the inode_lock twice?
>>
> 
> lockdep should catch that.
> 

hey that's a good idea...!  *sigh* sometimes I worry about myself... but 
hey at least I got it right. :)

=============================================
[ INFO: possible recursive locking detected ]
2.6.22-rc3 #8
---------------------------------------------
lt-fsstress/3285 is trying to acquire lock:
  (inode_lock){--..}, at: [<ffffffff802b0de9>] __mark_inode_dirty+0xe2/0x16c

but task is already holding lock:
  (inode_lock){--..}, at: [<ffffffff80316cc9>] 
_atomic_dec_and_lock+0x39/0x58

other info that might help us debug this:
3 locks held by lt-fsstress/3285:
  #0:  (&inode->i_mutex/1){--..}, at: [<ffffffff8029f262>] 
do_rmdir+0x7c/0xe3
  #1:  (&inode->i_mutex){--..}, at: [<ffffffff80462809>] 
mutex_lock+0x22/0x24
  #2:  (inode_lock){--..}, at: [<ffffffff80316cc9>] 
_atomic_dec_and_lock+0x39/0x58

stack backtrace:

Call Trace:
  [<ffffffff8024e1fc>] __lock_acquire+0x155/0xbaa
  [<ffffffff802b0de9>] __mark_inode_dirty+0xe2/0x16c
  [<ffffffff8024eccc>] lock_acquire+0x7b/0x9f
  [<ffffffff802b0de9>] __mark_inode_dirty+0xe2/0x16c
  [<ffffffff80463bc9>] _spin_lock+0x1e/0x28
  [<ffffffff802b0de9>] __mark_inode_dirty+0xe2/0x16c
  [<ffffffff882dc7cc>] :udf:udf_write_aext+0x101/0x11b
  [<ffffffff882e5992>] :udf:extent_trunc+0xd6/0x123
  [<ffffffff882e5ab9>] :udf:udf_truncate_tail_extent+0xda/0x171
  [<ffffffff882dfc5e>] :udf:udf_drop_inode+0x26/0x35
  [<ffffffff802a726d>] iput+0x74/0x76
  [<ffffffff802a4e9b>] dentry_iput+0xa0/0xb8
  [<ffffffff802a612a>] prune_dcache+0xa2/0x174
  [<ffffffff802a4f3c>] d_kill+0x21/0x43
  [<ffffffff802a5eef>] prune_one_dentry+0x3a/0xef
  [<ffffffff802a6175>] prune_dcache+0xed/0x174
  [<ffffffff802a6253>] shrink_dcache_parent+0x21/0x10e
  [<ffffffff8029becd>] dentry_unhash+0x26/0x84
  [<ffffffff8029d23c>] vfs_rmdir+0x88/0x117
  [<ffffffff8029f287>] do_rmdir+0xa1/0xe3
  [<ffffffff8020cf4b>] syscall_trace_enter+0x8d/0x8f
  [<ffffffff8029f300>] sys_rmdir+0x11/0x13
  [<ffffffff80209da5>] tracesys+0xdc/0xe1

-Eric

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Andrew Morton]

On Sat, 02 Jun 2007 00:17:51 -0500 Eric Sandeen <sandeen@sandeen.net> wrote:

> Andrew Morton wrote:
> > On Fri, 01 Jun 2007 17:37:49 -0500
> > Eric Sandeen <sandeen@sandeen.net> wrote:
> 
> >> going for the inode_lock twice?
> >>
> > 
> > lockdep should catch that.
> > 
> 
> hey that's a good idea...!  *sigh* sometimes I worry about myself... but 
> hey at least I got it right. :)
> 
> =============================================
> [ INFO: possible recursive locking detected ]
> 2.6.22-rc3 #8
> ---------------------------------------------
> lt-fsstress/3285 is trying to acquire lock:
>   (inode_lock){--..}, at: [<ffffffff802b0de9>] __mark_inode_dirty+0xe2/0x16c
> 
> but task is already holding lock:
>   (inode_lock){--..}, at: [<ffffffff80316cc9>] 
> _atomic_dec_and_lock+0x39/0x58
> 
> other info that might help us debug this:
> 3 locks held by lt-fsstress/3285:
>   #0:  (&inode->i_mutex/1){--..}, at: [<ffffffff8029f262>] 
> do_rmdir+0x7c/0xe3
>   #1:  (&inode->i_mutex){--..}, at: [<ffffffff80462809>] 
> mutex_lock+0x22/0x24
>   #2:  (inode_lock){--..}, at: [<ffffffff80316cc9>] 
> _atomic_dec_and_lock+0x39/0x58
> 
> stack backtrace:
> 
> Call Trace:
>   [<ffffffff8024e1fc>] __lock_acquire+0x155/0xbaa
>   [<ffffffff802b0de9>] __mark_inode_dirty+0xe2/0x16c
>   [<ffffffff8024eccc>] lock_acquire+0x7b/0x9f
>   [<ffffffff802b0de9>] __mark_inode_dirty+0xe2/0x16c
>   [<ffffffff80463bc9>] _spin_lock+0x1e/0x28
>   [<ffffffff802b0de9>] __mark_inode_dirty+0xe2/0x16c
>   [<ffffffff882dc7cc>] :udf:udf_write_aext+0x101/0x11b
>   [<ffffffff882e5992>] :udf:extent_trunc+0xd6/0x123
>   [<ffffffff882e5ab9>] :udf:udf_truncate_tail_extent+0xda/0x171
>   [<ffffffff882dfc5e>] :udf:udf_drop_inode+0x26/0x35
>   [<ffffffff802a726d>] iput+0x74/0x76
>   [<ffffffff802a4e9b>] dentry_iput+0xa0/0xb8
>   [<ffffffff802a612a>] prune_dcache+0xa2/0x174
>   [<ffffffff802a4f3c>] d_kill+0x21/0x43
>   [<ffffffff802a5eef>] prune_one_dentry+0x3a/0xef
>   [<ffffffff802a6175>] prune_dcache+0xed/0x174
>   [<ffffffff802a6253>] shrink_dcache_parent+0x21/0x10e
>   [<ffffffff8029becd>] dentry_unhash+0x26/0x84
>   [<ffffffff8029d23c>] vfs_rmdir+0x88/0x117
>   [<ffffffff8029f287>] do_rmdir+0xa1/0xe3
>   [<ffffffff8020cf4b>] syscall_trace_enter+0x8d/0x8f
>   [<ffffffff8029f300>] sys_rmdir+0x11/0x13
>   [<ffffffff80209da5>] tracesys+0xdc/0xe1
> 

Well.  Documentation/filesystems/Locking says

drop_inode:             no                              !!!inode_lock!!!

That patch is DOA, methinks.

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Andrew Morton - Fri, Jun 01, 2007 at 10:43:39PM -0700]
| On Sat, 02 Jun 2007 00:17:51 -0500 Eric Sandeen <sandeen@sandeen.net> wrote:
| 
| > Andrew Morton wrote:
| > > On Fri, 01 Jun 2007 17:37:49 -0500
| > > Eric Sandeen <sandeen@sandeen.net> wrote:
| > 
| > >> going for the inode_lock twice?
| > >>
| > > 
| > > lockdep should catch that.
| > > 
| > 
| > hey that's a good idea...!  *sigh* sometimes I worry about myself... but 
| > hey at least I got it right. :)
| > 
| > =============================================
| > [ INFO: possible recursive locking detected ]
| > 2.6.22-rc3 #8
| > ---------------------------------------------
| > lt-fsstress/3285 is trying to acquire lock:
| >   (inode_lock){--..}, at: [<ffffffff802b0de9>] __mark_inode_dirty+0xe2/0x16c
| > 
| > but task is already holding lock:
| >   (inode_lock){--..}, at: [<ffffffff80316cc9>] 
| > _atomic_dec_and_lock+0x39/0x58
| > 
| > other info that might help us debug this:
| > 3 locks held by lt-fsstress/3285:
| >   #0:  (&inode->i_mutex/1){--..}, at: [<ffffffff8029f262>] 
| > do_rmdir+0x7c/0xe3
| >   #1:  (&inode->i_mutex){--..}, at: [<ffffffff80462809>] 
| > mutex_lock+0x22/0x24
| >   #2:  (inode_lock){--..}, at: [<ffffffff80316cc9>] 
| > _atomic_dec_and_lock+0x39/0x58
| > 
| > stack backtrace:
| > 
| > Call Trace:
| >   [<ffffffff8024e1fc>] __lock_acquire+0x155/0xbaa
| >   [<ffffffff802b0de9>] __mark_inode_dirty+0xe2/0x16c
| >   [<ffffffff8024eccc>] lock_acquire+0x7b/0x9f
| >   [<ffffffff802b0de9>] __mark_inode_dirty+0xe2/0x16c
| >   [<ffffffff80463bc9>] _spin_lock+0x1e/0x28
| >   [<ffffffff802b0de9>] __mark_inode_dirty+0xe2/0x16c
| >   [<ffffffff882dc7cc>] :udf:udf_write_aext+0x101/0x11b
| >   [<ffffffff882e5992>] :udf:extent_trunc+0xd6/0x123
| >   [<ffffffff882e5ab9>] :udf:udf_truncate_tail_extent+0xda/0x171
| >   [<ffffffff882dfc5e>] :udf:udf_drop_inode+0x26/0x35
| >   [<ffffffff802a726d>] iput+0x74/0x76
| >   [<ffffffff802a4e9b>] dentry_iput+0xa0/0xb8
| >   [<ffffffff802a612a>] prune_dcache+0xa2/0x174
| >   [<ffffffff802a4f3c>] d_kill+0x21/0x43
| >   [<ffffffff802a5eef>] prune_one_dentry+0x3a/0xef
| >   [<ffffffff802a6175>] prune_dcache+0xed/0x174
| >   [<ffffffff802a6253>] shrink_dcache_parent+0x21/0x10e
| >   [<ffffffff8029becd>] dentry_unhash+0x26/0x84
| >   [<ffffffff8029d23c>] vfs_rmdir+0x88/0x117
| >   [<ffffffff8029f287>] do_rmdir+0xa1/0xe3
| >   [<ffffffff8020cf4b>] syscall_trace_enter+0x8d/0x8f
| >   [<ffffffff8029f300>] sys_rmdir+0x11/0x13
| >   [<ffffffff80209da5>] tracesys+0xdc/0xe1
| > 
| 
| Well.  Documentation/filesystems/Locking says
| 
| drop_inode:             no                              !!!inode_lock!!!
| 
| That patch is DOA, methinks.
| 

Andrew, what does it mean - "DOA"? Dead on arrival?

		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Andrew Morton]

On Sat, 2 Jun 2007 10:34:03 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> | That patch is DOA, methinks.
> | 
> 
> Andrew, what does it mean - "DOA"? Dead on arrival?

yes - I dropped it.

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Andrew Morton - Fri, Jun 01, 2007 at 11:54:22PM -0700]
| On Sat, 2 Jun 2007 10:34:03 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > | That patch is DOA, methinks.
| > | 
| > 
| > Andrew, what does it mean - "DOA"? Dead on arrival?
| 
| yes - I dropped it.
| 

But that could lead to rejection of my code-style-conversion patch...
Should I remake them?

Actually Jan was right, the current state of UDF (without his patches)
could lead to lost blocks and his patch must be just fixed I think.

		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Andrew Morton]

On Sat, 2 Jun 2007 10:59:23 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Andrew Morton - Fri, Jun 01, 2007 at 11:54:22PM -0700]
> | On Sat, 2 Jun 2007 10:34:03 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> | 
> | > | That patch is DOA, methinks.
> | > | 
> | > 
> | > Andrew, what does it mean - "DOA"? Dead on arrival?
> | 
> | yes - I dropped it.
> | 
> 
> But that could lead to rejection of my code-style-conversion patch...
> Should I remake them?

Actually I've rebuilt those patches four times already.  People keep
changing stuff.

> Actually Jan was right, the current state of UDF (without his patches)
> could lead to lost blocks and his patch must be just fixed I think.

sure.

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Andrew Morton - Sat, Jun 02, 2007 at 12:06:45AM -0700]
| On Sat, 2 Jun 2007 10:59:23 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > [Andrew Morton - Fri, Jun 01, 2007 at 11:54:22PM -0700]
| > | On Sat, 2 Jun 2007 10:34:03 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > | 
| > | > | That patch is DOA, methinks.
| > | > | 
| > | > 
| > | > Andrew, what does it mean - "DOA"? Dead on arrival?
| > | 
| > | yes - I dropped it.
| > | 
| > 
| > But that could lead to rejection of my code-style-conversion patch...
| > Should I remake them?
| 
| Actually I've rebuilt those patches four times already.  People keep
| changing stuff.
| 
| > Actually Jan was right, the current state of UDF (without his patches)
| > could lead to lost blocks and his patch must be just fixed I think.
| 
| sure.
| 

Andrew, you know I've been trying to reproduce Eric's lockup case almost
two hour and still can't reach it. All manupulation I've done to UDF didn't
lead to lockup. Moreover, I've added debug print for UDF module and here is
the results (for single drop_inode call):

	[12063.897000] UDF: udf_drop_inode:105 --> udf_drop_inode --> inode->i_count: 0
	[12063.897000] UDF: udf_drop_inode:107 udf_drop_inode -> discard_prealloc
	[12063.897000] UDF: udf_discard_prealloc:136 udf_discard_prealloc
	[12063.897000] UDF: udf_truncate_tail_extent:84 udf_truncate_tail_extent
	[12063.897000] UDF: udf_truncate_extents:194 udf_truncate_extents -->
	[12063.897000] UDF: extent_trunc:38 --->
	[12063.897000] UDF: extent_trunc:54 call to udf_write_aext
	[12063.897000] UDF: udf_write_aext:1843 udf_write_aext
	[12063.897000] UDF: udf_write_aext:1846 dont has epos->bh
	[12063.897000] UDF: udf_write_aext:1866 ICBTAG_FLAG_AD_LONG
--->	[12063.897000] UDF: udf_write_aext:1893 ---> gotcha ---> call mark_inode_dirty
--->	[12063.897000] UDF: extent_trunc:59 --> gotcha --> call mark_inode_dirty
	[12063.897000] UDF: extent_trunc:68 <---
--->	[12063.897000] UDF: udf_truncate_extents:282 call mark_inode_dirty
	[12063.897000] UDF: udf_truncate_extents:330 udf_truncate_extents <--
	[12063.897000] UDF: udf_drop_inode:115 <-- udf_drop_inode <--

As you may see, mark_inode_dirty is called several time and no locking happened.
Maybe I should use some test utils?

		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Andrew Morton]

On Sat, 2 Jun 2007 18:06:19 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Andrew Morton - Sat, Jun 02, 2007 at 12:06:45AM -0700]
> | On Sat, 2 Jun 2007 10:59:23 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> | 
> | > [Andrew Morton - Fri, Jun 01, 2007 at 11:54:22PM -0700]
> | > | On Sat, 2 Jun 2007 10:34:03 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> | > | 
> | > | > | That patch is DOA, methinks.
> | > | > | 
> | > | > 
> | > | > Andrew, what does it mean - "DOA"? Dead on arrival?
> | > | 
> | > | yes - I dropped it.
> | > | 
> | > 
> | > But that could lead to rejection of my code-style-conversion patch...
> | > Should I remake them?
> | 
> | Actually I've rebuilt those patches four times already.  People keep
> | changing stuff.
> | 
> | > Actually Jan was right, the current state of UDF (without his patches)
> | > could lead to lost blocks and his patch must be just fixed I think.
> | 
> | sure.
> | 
> 
> Andrew, you know I've been trying to reproduce Eric's lockup case almost
> two hour and still can't reach it. All manupulation I've done to UDF didn't
> lead to lockup. Moreover, I've added debug print for UDF module and here is
> the results (for single drop_inode call):
> 
> 	[12063.897000] UDF: udf_drop_inode:105 --> udf_drop_inode --> inode->i_count: 0
> 	[12063.897000] UDF: udf_drop_inode:107 udf_drop_inode -> discard_prealloc
> 	[12063.897000] UDF: udf_discard_prealloc:136 udf_discard_prealloc
> 	[12063.897000] UDF: udf_truncate_tail_extent:84 udf_truncate_tail_extent
> 	[12063.897000] UDF: udf_truncate_extents:194 udf_truncate_extents -->
> 	[12063.897000] UDF: extent_trunc:38 --->
> 	[12063.897000] UDF: extent_trunc:54 call to udf_write_aext
> 	[12063.897000] UDF: udf_write_aext:1843 udf_write_aext
> 	[12063.897000] UDF: udf_write_aext:1846 dont has epos->bh
> 	[12063.897000] UDF: udf_write_aext:1866 ICBTAG_FLAG_AD_LONG
> --->	[12063.897000] UDF: udf_write_aext:1893 ---> gotcha ---> call mark_inode_dirty
> --->	[12063.897000] UDF: extent_trunc:59 --> gotcha --> call mark_inode_dirty
> 	[12063.897000] UDF: extent_trunc:68 <---
> --->	[12063.897000] UDF: udf_truncate_extents:282 call mark_inode_dirty
> 	[12063.897000] UDF: udf_truncate_extents:330 udf_truncate_extents <--
> 	[12063.897000] UDF: udf_drop_inode:115 <-- udf_drop_inode <--
> 
> As you may see, mark_inode_dirty is called several time and no locking happened.
> Maybe I should use some test utils?
> 

Silly question: you _do_ have CONFIG_SMP=y, yes?

And did you enable lockdep?

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Andrew Morton - Sat, Jun 02, 2007 at 10:32:03AM -0700]
| On Sat, 2 Jun 2007 18:06:19 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > [Andrew Morton - Sat, Jun 02, 2007 at 12:06:45AM -0700]
| > | On Sat, 2 Jun 2007 10:59:23 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > | 
| > | > [Andrew Morton - Fri, Jun 01, 2007 at 11:54:22PM -0700]
| > | > | On Sat, 2 Jun 2007 10:34:03 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > | > | 
| > | > | > | That patch is DOA, methinks.
| > | > | > | 
| > | > | > 
| > | > | > Andrew, what does it mean - "DOA"? Dead on arrival?
| > | > | 
| > | > | yes - I dropped it.
| > | > | 
| > | > 
| > | > But that could lead to rejection of my code-style-conversion patch...
| > | > Should I remake them?
| > | 
| > | Actually I've rebuilt those patches four times already.  People keep
| > | changing stuff.
| > | 
| > | > Actually Jan was right, the current state of UDF (without his patches)
| > | > could lead to lost blocks and his patch must be just fixed I think.
| > | 
| > | sure.
| > | 
| > 
| > Andrew, you know I've been trying to reproduce Eric's lockup case almost
| > two hour and still can't reach it. All manupulation I've done to UDF didn't
| > lead to lockup. Moreover, I've added debug print for UDF module and here is
| > the results (for single drop_inode call):
| > 
| > 	[12063.897000] UDF: udf_drop_inode:105 --> udf_drop_inode --> inode->i_count: 0
| > 	[12063.897000] UDF: udf_drop_inode:107 udf_drop_inode -> discard_prealloc
| > 	[12063.897000] UDF: udf_discard_prealloc:136 udf_discard_prealloc
| > 	[12063.897000] UDF: udf_truncate_tail_extent:84 udf_truncate_tail_extent
| > 	[12063.897000] UDF: udf_truncate_extents:194 udf_truncate_extents -->
| > 	[12063.897000] UDF: extent_trunc:38 --->
| > 	[12063.897000] UDF: extent_trunc:54 call to udf_write_aext
| > 	[12063.897000] UDF: udf_write_aext:1843 udf_write_aext
| > 	[12063.897000] UDF: udf_write_aext:1846 dont has epos->bh
| > 	[12063.897000] UDF: udf_write_aext:1866 ICBTAG_FLAG_AD_LONG
| > --->	[12063.897000] UDF: udf_write_aext:1893 ---> gotcha ---> call mark_inode_dirty
| > --->	[12063.897000] UDF: extent_trunc:59 --> gotcha --> call mark_inode_dirty
| > 	[12063.897000] UDF: extent_trunc:68 <---
| > --->	[12063.897000] UDF: udf_truncate_extents:282 call mark_inode_dirty
| > 	[12063.897000] UDF: udf_truncate_extents:330 udf_truncate_extents <--
| > 	[12063.897000] UDF: udf_drop_inode:115 <-- udf_drop_inode <--
| > 
| > As you may see, mark_inode_dirty is called several time and no locking happened.
| > Maybe I should use some test utils?
| > 
| 
| Silly question: you _do_ have CONFIG_SMP=y, yes?
| 
Oh, no I don't :( So the problem is in kernel sync (as I thought)...
damn... I have to rebuild my kernel... but hold on - my machine has only
one CPU ;)

| And did you enable lockdep?
| 
Yes

So the problem is 'cause of mark_inode_dirty may sleep? Right?
So only thing to be checked is lock_kernel I think

		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Andrew Morton]

On Sat, 2 Jun 2007 22:57:07 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Andrew Morton - Sat, Jun 02, 2007 at 10:32:03AM -0700]
> | On Sat, 2 Jun 2007 18:06:19 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> | 
> | > [Andrew Morton - Sat, Jun 02, 2007 at 12:06:45AM -0700]
> | > | On Sat, 2 Jun 2007 10:59:23 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> | > | 
> | > | > [Andrew Morton - Fri, Jun 01, 2007 at 11:54:22PM -0700]
> | > | > | On Sat, 2 Jun 2007 10:34:03 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> | > | > | 
> | > | > | > | That patch is DOA, methinks.
> | > | > | > | 
> | > | > | > 
> | > | > | > Andrew, what does it mean - "DOA"? Dead on arrival?
> | > | > | 
> | > | > | yes - I dropped it.
> | > | > | 
> | > | > 
> | > | > But that could lead to rejection of my code-style-conversion patch...
> | > | > Should I remake them?
> | > | 
> | > | Actually I've rebuilt those patches four times already.  People keep
> | > | changing stuff.
> | > | 
> | > | > Actually Jan was right, the current state of UDF (without his patches)
> | > | > could lead to lost blocks and his patch must be just fixed I think.
> | > | 
> | > | sure.
> | > | 
> | > 
> | > Andrew, you know I've been trying to reproduce Eric's lockup case almost
> | > two hour and still can't reach it. All manupulation I've done to UDF didn't
> | > lead to lockup. Moreover, I've added debug print for UDF module and here is
> | > the results (for single drop_inode call):
> | > 
> | > 	[12063.897000] UDF: udf_drop_inode:105 --> udf_drop_inode --> inode->i_count: 0
> | > 	[12063.897000] UDF: udf_drop_inode:107 udf_drop_inode -> discard_prealloc
> | > 	[12063.897000] UDF: udf_discard_prealloc:136 udf_discard_prealloc
> | > 	[12063.897000] UDF: udf_truncate_tail_extent:84 udf_truncate_tail_extent
> | > 	[12063.897000] UDF: udf_truncate_extents:194 udf_truncate_extents -->
> | > 	[12063.897000] UDF: extent_trunc:38 --->
> | > 	[12063.897000] UDF: extent_trunc:54 call to udf_write_aext
> | > 	[12063.897000] UDF: udf_write_aext:1843 udf_write_aext
> | > 	[12063.897000] UDF: udf_write_aext:1846 dont has epos->bh
> | > 	[12063.897000] UDF: udf_write_aext:1866 ICBTAG_FLAG_AD_LONG
> | > --->	[12063.897000] UDF: udf_write_aext:1893 ---> gotcha ---> call mark_inode_dirty
> | > --->	[12063.897000] UDF: extent_trunc:59 --> gotcha --> call mark_inode_dirty
> | > 	[12063.897000] UDF: extent_trunc:68 <---
> | > --->	[12063.897000] UDF: udf_truncate_extents:282 call mark_inode_dirty
> | > 	[12063.897000] UDF: udf_truncate_extents:330 udf_truncate_extents <--
> | > 	[12063.897000] UDF: udf_drop_inode:115 <-- udf_drop_inode <--
> | > 
> | > As you may see, mark_inode_dirty is called several time and no locking happened.
> | > Maybe I should use some test utils?
> | > 
> | 
> | Silly question: you _do_ have CONFIG_SMP=y, yes?
> | 
> Oh, no I don't :( So the problem is in kernel sync (as I thought)...
> damn... I have to rebuild my kernel... but hold on - my machine has only
> one CPU ;)

You should be able to run an SMP kernel on a single-CPU machine.

> | And did you enable lockdep?
> | 
> Yes
> 
> So the problem is 'cause of mark_inode_dirty may sleep? Right?
> So only thing to be checked is lock_kernel I think

No, the problem is that the patch caused the kernel to take inode_lock
within the newly-added drop_inode(), btu drop_inode() is already called
under inode_lock.

It has nothing to do with lock_kernel() and it has nothing to do with
sleeping.

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Andrew Morton - Sat, Jun 02, 2007 at 12:16:16PM -0700]
[...snip...]
| 
| No, the problem is that the patch caused the kernel to take inode_lock
| within the newly-added drop_inode(), btu drop_inode() is already called
| under inode_lock.
| 
| It has nothing to do with lock_kernel() and it has nothing to do with
| sleeping.
| 

Andrew, the only call that could leading to subseq. inode_lock lock
is mark_inode_dirty() I guess (and that is snown by Eric's dump)
but as I shown you in my dbg print without SMP it's OK. So
is it SMP who lead to lock? How it depends on it? (I understand
that is a stupid question for you but if you have time explain
me this please ;)

		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Andrew Morton]

On Sun, 3 Jun 2007 00:01:46 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Andrew Morton - Sat, Jun 02, 2007 at 12:16:16PM -0700]
> [...snip...]
> | 
> | No, the problem is that the patch caused the kernel to take inode_lock
> | within the newly-added drop_inode(), btu drop_inode() is already called
> | under inode_lock.
> | 
> | It has nothing to do with lock_kernel() and it has nothing to do with
> | sleeping.
> | 
> 
> Andrew, the only call that could leading to subseq. inode_lock lock
> is mark_inode_dirty() I guess (and that is snown by Eric's dump)
> but as I shown you in my dbg print without SMP it's OK. So
> is it SMP who lead to lock? How it depends on it? (I understand
> that is a stupid question for you but if you have time explain
> me this please ;)
> 

When CONFIG_SMP=n, spin_lock() is a no-op.  (Except with CONFIG_PREEMPT=y,
in which case spin_lock() will disable kernel preemption on SMP and non-SMP
kernels)

When CONFIG_SMP=y, spin_lock() really does take a lock.  But if this thread
already holds this lock, we'll deadlock.

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Andrew Morton - Sat, Jun 02, 2007 at 03:49:42PM -0700]
| On Sun, 3 Jun 2007 00:01:46 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > [Andrew Morton - Sat, Jun 02, 2007 at 12:16:16PM -0700]
| > [...snip...]
| > | 
| > | No, the problem is that the patch caused the kernel to take inode_lock
| > | within the newly-added drop_inode(), btu drop_inode() is already called
| > | under inode_lock.
| > | 
| > | It has nothing to do with lock_kernel() and it has nothing to do with
| > | sleeping.
| > | 
| > 
| > Andrew, the only call that could leading to subseq. inode_lock lock
| > is mark_inode_dirty() I guess (and that is snown by Eric's dump)
| > but as I shown you in my dbg print without SMP it's OK. So
| > is it SMP who lead to lock? How it depends on it? (I understand
| > that is a stupid question for you but if you have time explain
| > me this please ;)
| > 
| 
| When CONFIG_SMP=n, spin_lock() is a no-op.  (Except with CONFIG_PREEMPT=y,
| in which case spin_lock() will disable kernel preemption on SMP and non-SMP
| kernels)
| 
| When CONFIG_SMP=y, spin_lock() really does take a lock.  But if this thread
| already holds this lock, we'll deadlock.
| 

Thanks, Andrew. So the reason that raises lock problem is the calling of
mark_inode_dirty() inside drop_inode() (by indirection). And I see two way
of solution:

- or check for inode->i_count at each mark_inode_dirty that being called
  after drop_inode

		if (inode->i_count > 0)
				mark_inode_dirty()

- or wrap mark_inode_dirty as

	udf_mark_inode_dirty()
	{
		if (inode->i_count > 0)
			mark_inode_dirty();
	}

	and replace all mark_inode_dirty -> udf_mark_inode_dirty

Your thoughts?

		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Cyrill Gorcunov - Sun, Jun 03, 2007 at 10:28:40AM +0400]
| [Andrew Morton - Sat, Jun 02, 2007 at 03:49:42PM -0700]
| | On Sun, 3 Jun 2007 00:01:46 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| | 
| | > [Andrew Morton - Sat, Jun 02, 2007 at 12:16:16PM -0700]
| | > [...snip...]
| | > | 
| | > | No, the problem is that the patch caused the kernel to take inode_lock
| | > | within the newly-added drop_inode(), btu drop_inode() is already called
| | > | under inode_lock.
| | > | 
| | > | It has nothing to do with lock_kernel() and it has nothing to do with
| | > | sleeping.
| | > | 
| | > 
| | > Andrew, the only call that could leading to subseq. inode_lock lock
| | > is mark_inode_dirty() I guess (and that is snown by Eric's dump)
| | > but as I shown you in my dbg print without SMP it's OK. So
| | > is it SMP who lead to lock? How it depends on it? (I understand
| | > that is a stupid question for you but if you have time explain
| | > me this please ;)
| | > 
| | 
| | When CONFIG_SMP=n, spin_lock() is a no-op.  (Except with CONFIG_PREEMPT=y,
| | in which case spin_lock() will disable kernel preemption on SMP and non-SMP
| | kernels)
| | 
| | When CONFIG_SMP=y, spin_lock() really does take a lock.  But if this thread
| | already holds this lock, we'll deadlock.
| | 
| 
| Thanks, Andrew. So the reason that raises lock problem is the calling of
| mark_inode_dirty() inside drop_inode() (by indirection). And I see two way
| of solution:
| 
| - or check for inode->i_count at each mark_inode_dirty that being called
|   after drop_inode
| 
| 		if (inode->i_count > 0)
| 				mark_inode_dirty()
| 
| - or wrap mark_inode_dirty as
| 
| 	udf_mark_inode_dirty()
| 	{
- 		if (inode->i_count > 0)
+		if (atomic_read(&inode->i_count) > 0)
| 			mark_inode_dirty();
| 	}
| 
| 	and replace all mark_inode_dirty -> udf_mark_inode_dirty
| 
| Your thoughts?
| 
| 		Cyrill
| 


		Cyrill

Re: [PATCH 2/2] Fix possible leakage of blocks in UDF

[Cyrill Gorcunov]

[Jan Kara - Fri, Jun 01, 2007 at 11:10:36PM +0200]
| On Wed 30-05-07 16:46:28, Eric Sandeen wrote:
| > Jan Kara wrote:
| > >   Hello,
| > > 
| > > On Thu 24-05-07 19:05:54, Jan Kara wrote:
| > >>   Hello,
| > >>
| > >>   attached is a patch that fixes possible leakage of free blocks / use of
| > >> free blocks in UDF (which spilled nice assertion failures I've added in my
| > >> first round of patches). More details in the changelog. Andrew, please apply.
| > >> Both changes have survived some time of fsx and fsstress testing so they
| > >> should be reasonably safe.
| > >   Sorry for replying to myself but this patch had a minor problem of
| > > printing some bogus warnings when directories were deleted (I wonder why
| > > fsstress didn't find it). Attached is a new version of the patch without
| > > this problem.
| > 
| > Jan, something seems busted here.  I'm getting lockups when testing udf
| > on a single cpu with this last patch in place...
|   Hmm, strange, I was also testing on UP and without problems. And I didn't
| change any locking...
| 
| > I think it's the BKL stumbling on itself.
| > 
| > for example...
| > 
| > static int udf_symlink(struct inode * dir, struct dentry * dentry, const
| > char * symname)
| > {
| > ...
| >         lock_kernel();
| > ...
| > out:
| >         unlock_kernel();
| >         return err;
| > 
| > out_no_entry:
| >         inode_dec_link_count(inode);
| >         iput(inode);
| >         goto out;
| > }
| > 
| > but iput goes
| > iput->iput_final->drop_inode->udf_drop_inode->lock_kernel() again
|   As Andrew already wrote, BKL is free to recurse...
| 
| > looking for the right way around it but figured I'd ping you early :)
|   Thanks for info - I'm now mostly out of email for a few days but I'll
| have a look at it as soon as I return.
| 
| 								Honza
| -- 
| Jan Kara <jack@suse.cz>
| SuSE CR Labs
| 

Hi Jan,

why can't we combine udf_delete_inode() with udf_drop_inode()?
It'll avoid deadlock.

		Cyrill

Re: [PATCH 1/2] Fix possible UDF data corruption

[Cyrill Gorcunov]

[Jan Kara - Thu, May 24, 2007 at 06:59:35PM +0200]
|   Hi Andrew,
| 
|   attached patch fixes possible data corruption in UDF - this bug was actually
| introduced by one of my fixes :-( and should (if possible) go to Linus before
| 2.6.22 is out (that's why I'm diffing against Linus's tree and not the
| latest changes in -mm tree)... Thanks.
| 
| 								Honza
| 
| -- 
| Jan Kara <jack@suse.cz>
| SuSE CR Labs

Jan should I wait until Andrew has your patches included and
only then (having taken into account your patches) produce my
conversion?

		Cyrill

Re: [PATCH 1/2] Fix possible UDF data corruption

[Andrew Morton]

On Thu, 24 May 2007 21:20:17 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Jan Kara - Thu, May 24, 2007 at 06:59:35PM +0200]
> |   Hi Andrew,
> | 
> |   attached patch fixes possible data corruption in UDF - this bug was actually
> | introduced by one of my fixes :-( and should (if possible) go to Linus before
> | 2.6.22 is out (that's why I'm diffing against Linus's tree and not the
> | latest changes in -mm tree)... Thanks.
> | 
> | 								Honza
> | 
> | -- 
> | Jan Kara <jack@suse.cz>
> | SuSE CR Labs
> 
> Jan should I wait until Andrew has your patches included and
> only then (having taken into account your patches) produce my
> conversion?
> 

yes please - bugfixes come first.

Re: [PATCH 1/2] Fix possible UDF data corruption

[Cyrill Gorcunov]

[Andrew Morton - Thu, May 24, 2007 at 11:35:50AM -0700]
| On Thu, 24 May 2007 21:20:17 +0400
| Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > [Jan Kara - Thu, May 24, 2007 at 06:59:35PM +0200]
| > |   Hi Andrew,
| > | 
| > |   attached patch fixes possible data corruption in UDF - this bug was actually
| > | introduced by one of my fixes :-( and should (if possible) go to Linus before
| > | 2.6.22 is out (that's why I'm diffing against Linus's tree and not the
| > | latest changes in -mm tree)... Thanks.
| > | 
| > | 								Honza
| > | 
| > | -- 
| > | Jan Kara <jack@suse.cz>
| > | SuSE CR Labs
| > 
| > Jan should I wait until Andrew has your patches included and
| > only then (having taken into account your patches) produce my
| > conversion?
| > 
| 
| yes please - bugfixes come first.
| 

ok

		Cyrill

Re: [PATCH 1/2] Fix possible UDF data corruption

[Cyrill Gorcunov]

[Andrew Morton - Thu, May 24, 2007 at 11:35:50AM -0700]
| On Thu, 24 May 2007 21:20:17 +0400
| Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > [Jan Kara - Thu, May 24, 2007 at 06:59:35PM +0200]
| > |   Hi Andrew,
| > | 
| > |   attached patch fixes possible data corruption in UDF - this bug was actually
| > | introduced by one of my fixes :-( and should (if possible) go to Linus before
| > | 2.6.22 is out (that's why I'm diffing against Linus's tree and not the
| > | latest changes in -mm tree)... Thanks.
| > | 
| > | 								Honza
| > | 
| > | -- 
| > | Jan Kara <jack@suse.cz>
| > | SuSE CR Labs
| > 
| > Jan should I wait until Andrew has your patches included and
| > only then (having taken into account your patches) produce my
| > conversion?
| > 
| 
| yes please - bugfixes come first.
| 

Andrew, so could I assume that these patches are in your -mm tree
to be able to work on UDF style conversion?

		Cyrill

Re: [PATCH 1/2] Fix possible UDF data corruption

[Andrew Morton]

On Thu, 24 May 2007 23:23:21 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Andrew Morton - Thu, May 24, 2007 at 11:35:50AM -0700]
> | On Thu, 24 May 2007 21:20:17 +0400
> | Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> | 
> | > [Jan Kara - Thu, May 24, 2007 at 06:59:35PM +0200]
> | > |   Hi Andrew,
> | > | 
> | > |   attached patch fixes possible data corruption in UDF - this bug was actually
> | > | introduced by one of my fixes :-( and should (if possible) go to Linus before
> | > | 2.6.22 is out (that's why I'm diffing against Linus's tree and not the
> | > | latest changes in -mm tree)... Thanks.
> | > | 
> | > | 								Honza
> | > | 
> | > | -- 
> | > | Jan Kara <jack@suse.cz>
> | > | SuSE CR Labs
> | > 
> | > Jan should I wait until Andrew has your patches included and
> | > only then (having taken into account your patches) produce my
> | > conversion?
> | > 
> | 
> | yes please - bugfixes come first.
> | 
> 
> Andrew, so could I assume that these patches are in your -mm tree
> to be able to work on UDF style conversion?
> 

Sure.  If something breaks, I'll fix it up.

If you take the two-patch approach (first patch is Lindent, second patch is
post-Lindent fixups) then it becomes easy at this end: the first patch is
the big one and if it breaks, I just re-run Lindent and regenerate it.

Re: [PATCH 1/2] Fix possible UDF data corruption

[Cyrill Gorcunov]

[Andrew Morton - Thu, May 24, 2007 at 12:36:21PM -0700]
| On Thu, 24 May 2007 23:23:21 +0400
| Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > [Andrew Morton - Thu, May 24, 2007 at 11:35:50AM -0700]
| > | On Thu, 24 May 2007 21:20:17 +0400
| > | Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > | 
| > | > [Jan Kara - Thu, May 24, 2007 at 06:59:35PM +0200]
| > | > |   Hi Andrew,
| > | > | 
| > | > |   attached patch fixes possible data corruption in UDF - this bug was actually
| > | > | introduced by one of my fixes :-( and should (if possible) go to Linus before
| > | > | 2.6.22 is out (that's why I'm diffing against Linus's tree and not the
| > | > | latest changes in -mm tree)... Thanks.
| > | > | 
| > | > | 								Honza
| > | > | 
| > | > | -- 
| > | > | Jan Kara <jack@suse.cz>
| > | > | SuSE CR Labs
| > | > 
| > | > Jan should I wait until Andrew has your patches included and
| > | > only then (having taken into account your patches) produce my
| > | > conversion?
| > | > 
| > | 
| > | yes please - bugfixes come first.
| > | 
| > 
| > Andrew, so could I assume that these patches are in your -mm tree
| > to be able to work on UDF style conversion?
| > 
| 
| Sure.  If something breaks, I'll fix it up.
| 
| If you take the two-patch approach (first patch is Lindent, second patch is
| post-Lindent fixups) then it becomes easy at this end: the first patch is
| the big one and if it breaks, I just re-run Lindent and regenerate it.
| 

OK

		Cyrill