UDF: fix deadlock on inode being dropped

This patch prevents from deadlock on inode being dropped. The deadlock is caused by inderect call of mark_inode_dirty() within udf_drop_inode() but inode lock is already kept by the kernel. So moving code from udf_drop_inode() to udf_delete_inode() we save its functionality and avoid deadlock.

---
Andrew the patch is over Jan's two patches you have dropped. AFAIS a such code moving is quite safe but I'm waiting for Jan's comments anyway.

 fs/udf/inode.c |   20 ++++++++------------
 fs/udf/super.c |    1 -
 2 files changed, 8 insertions(+), 13 deletions(-)

index 9fcd76c..ae56f25 100644
@@ -83,6 +83,14 @@ static int udf_get_block(struct inode *, sector_t, struct buffer_head *, int);
  */
 void udf_delete_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();
+	}
+
 	truncate_inode_pages(&inode->i_data, 0);
 
 	if (is_bad_inode(inode))

@@ -102,18 +110,6 @@ no_delete:
 	clear_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));

index b02d2c2..deb0d29 100644
@@ -166,7 +166,6 @@ static const struct super_operations udf_sb_ops = {
 	.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,

Hi Cyrill!

The patch is wrong. You cannot truncate the extent just in delete_inode.
That would lead to inodes with untruncated last extent on disk after unmounting, which is forbidden in the specification. You need to truncate the last extent whenever inode is being removed from memory or something like that... I'm already thinking how to do it and avoid calling mark_inode_dirty()...

Honza

Jan Kara <[email protected]>
SuSE CR Labs

[Jan Kara - Thu, Jun 07, 2007 at 11:36:07AM +0200]

|   Hi Cyrill!
| 
| On Wed 06-06-07 21:53:51, Cyrill Gorcunov wrote:
| > This patch prevents from deadlock on inode being dropped.
| > The deadlock is caused by inderect call of mark_inode_dirty()
| > within udf_drop_inode() but inode lock is already kept
| > by the kernel. So moving code from udf_drop_inode() to
| > udf_delete_inode() we save its functionality and avoid
| > deadlock.
|   The patch is wrong. You cannot truncate the extent just in delete_inode.
| That would lead to inodes with untruncated last extent on disk after
| unmounting, which is forbidden in the specification. You need to truncate
| the last extent whenever inode is being removed from memory or something
| like that... I'm already thinking how to do it and avoid calling
| mark_inode_dirty()...
| 
| 
| 							Honza
| -- 
| Jan Kara <[email protected]>
| SuSE CR Labs
| 

Arh, thanks... Jan, actually the reason I've moved the code into 'delete' section was that I found no reasonable difference for our case between 'drop' and 'delete'. Moreover, by seeing into VFS code the only diff between 'drop' and 'delete' is that
inside generic_delete_inode() a few inode structure elements are being destroyed and then our udf_drop_inode is called. So assuming, that you're right in drop_inode I've code just moved to 'delete' section.

Cyrill

The difference is that udf_delete_inode() is called only when inode has
i_nlink == 0 and thus it's being deleted on disk. udf_drop_inode() is called whenever inode is removed from memory which is what we want.
I'm already testing a patch which should fix the problem...

Honza

Jan Kara <[email protected]>
SuSE CR Labs

[Jan Kara - Thu, Jun 07, 2007 at 04:41:21PM +0200]

| On Thu 07-06-07 17:54:58, Cyrill Gorcunov wrote:
| > [Jan Kara - Thu, Jun 07, 2007 at 11:36:07AM +0200]
| > |   Hi Cyrill!
| > | 
| > | On Wed 06-06-07 21:53:51, Cyrill Gorcunov wrote:
| > | > This patch prevents from deadlock on inode being dropped.
| > | > The deadlock is caused by inderect call of mark_inode_dirty()
| > | > within udf_drop_inode() but inode lock is already kept
| > | > by the kernel. So moving code from udf_drop_inode() to
| > | > udf_delete_inode() we save its functionality and avoid
| > | > deadlock.
| > |   The patch is wrong. You cannot truncate the extent just in delete_inode.
| > | That would lead to inodes with untruncated last extent on disk after
| > | unmounting, which is forbidden in the specification. You need to truncate
| > | the last extent whenever inode is being removed from memory or something
| > | like that... I'm already thinking how to do it and avoid calling
| > | mark_inode_dirty()...
| > | 
| > 
| > Arh, thanks... Jan, actually the reason I've moved the code into
| > 'delete' section was that I found no reasonable difference for our
| > case between 'drop' and 'delete'. Moreover, by seeing into VFS code
| > the only diff between 'drop' and 'delete' is that
| > inside generic_delete_inode() a few inode structure elements
| > are being destroyed and then our udf_drop_inode is called. So assuming,
| > that you're right in drop_inode I've code just moved to 'delete' section.
|   The difference is that udf_delete_inode() is called only when inode has
| i_nlink == 0 and thus it's being deleted on disk. udf_drop_inode() is
| called whenever inode is removed from memory which is what we want.
|   I'm already testing a patch which should fix the problem...
| 
| 								Honza
| -- 
| Jan Kara <[email protected]>
| SuSE CR Labs
| 

Thanks for eplanation, Jan

Cyrill

[Jan Kara - Thu, Jun 07, 2007 at 04:41:21PM +0200]

| On Thu 07-06-07 17:54:58, Cyrill Gorcunov wrote:
| > [Jan Kara - Thu, Jun 07, 2007 at 11:36:07AM +0200]
| > |   Hi Cyrill!
| > | 
| > | On Wed 06-06-07 21:53:51, Cyrill Gorcunov wrote:
| > | > This patch prevents from deadlock on inode being dropped.
| > | > The deadlock is caused by inderect call of mark_inode_dirty()
| > | > within udf_drop_inode() but inode lock is already kept
| > | > by the kernel. So moving code from udf_drop_inode() to
| > | > udf_delete_inode() we save its functionality and avoid
| > | > deadlock.
| > |   The patch is wrong. You cannot truncate the extent just in delete_inode.
| > | That would lead to inodes with untruncated last extent on disk after
| > | unmounting, which is forbidden in the specification. You need to truncate
| > | the last extent whenever inode is being removed from memory or something
| > | like that... I'm already thinking how to do it and avoid calling
| > | mark_inode_dirty()...
| > | 
| > 
| > Arh, thanks... Jan, actually the reason I've moved the code into
| > 'delete' section was that I found no reasonable difference for our
| > case between 'drop' and 'delete'. Moreover, by seeing into VFS code
| > the only diff between 'drop' and 'delete' is that
| > inside generic_delete_inode() a few inode structure elements
| > are being destroyed and then our udf_drop_inode is called. So assuming,
| > that you're right in drop_inode I've code just moved to 'delete' section.
|   The difference is that udf_delete_inode() is called only when inode has
| i_nlink == 0 and thus it's being deleted on disk. udf_drop_inode() is
| called whenever inode is removed from memory which is what we want.
|   I'm already testing a patch which should fix the problem...
| 
| 								Honza
| -- 
| Jan Kara <[email protected]>
| SuSE CR Labs
| 

Hi Jan,

how your progress? Could I help with something?

Cyrill

Eric has run his UDF test suite on it and it seems to survive fine so I'm
going to submit the patch to Andrew in a while.

Honza

Jan Kara <[email protected]>
SuSE CR Labs