[PATCH] UDF: fix deadlock on inode being dropped

[PATCH] UDF: fix deadlock on inode being dropped

[Cyrill Gorcunov]

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.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
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(-)


diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 9fcd76c..ae56f25 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -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));
diff --git a/fs/udf/super.c b/fs/udf/super.c
index b02d2c2..deb0d29 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -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,

Re: [PATCH] UDF: fix deadlock on inode being dropped

[Jan Kara]

  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 <jack@suse.cz>
SuSE CR Labs

Re: [PATCH] UDF: fix deadlock on inode being dropped

[Cyrill Gorcunov]

[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 <jack@suse.cz>
| 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

Re: [PATCH] UDF: fix deadlock on inode being dropped

[Jan Kara]

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 <jack@suse.cz>
SuSE CR Labs

Re: [PATCH] UDF: fix deadlock on inode being dropped

[Cyrill Gorcunov]

[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 <jack@suse.cz>
| SuSE CR Labs
| 

Thanks for eplanation, Jan

		Cyrill

Re: [PATCH] UDF: fix deadlock on inode being dropped

[Cyrill Gorcunov]

[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 <jack@suse.cz>
| SuSE CR Labs
| 

Hi Jan,

how your progress? Could I help with something?

		Cyrill

Re: [PATCH] UDF: fix deadlock on inode being dropped

[Jan Kara]

On Sat 09-06-07 17:35:36, Cyrill Gorcunov wrote:
> [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...
> 
> how your progress? Could I help with something?
  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 <jack@suse.cz>
SuSE CR Labs