[PATCH] ext4: Check io list state and avoid an unnecessary mutex_lock in ext4_end_io_work.

[PATCH] ext4: Check io list state and avoid an unnecessary mutex_lock in ext4_end_io_work.

[Tao Ma]

From: Tao Ma <boyu.mt@taobao.com>

When we finish the end io work in ext4_flush_completed_IO, we take
the io work away from the list, but don't free it. Then in the workqueue,
we can check the list state and then avoid the extra work if it is also
done. It is good, but we check list state in ext4_end_io_nolock with i_mutex held
instead of the spin_lock in other places. This is wrong.

So check the state within spin_lock and another side effect is that the heavy extra
mutex_lock can be avoided.

Cc: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Tao Ma <boyu.mt@taobao.com>
---
 fs/ext4/page-io.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 92f38ee..f6b40f1 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -100,9 +100,6 @@ int ext4_end_io_nolock(ext4_io_end_t *io)
 		   "list->prev 0x%p\n",
 		   io, inode->i_ino, io->list.next, io->list.prev);
 
-	if (list_empty(&io->list))
-		return ret;
-
 	if (!(io->flag & EXT4_IO_END_UNWRITTEN))
 		return ret;
 
@@ -142,6 +139,13 @@ static void ext4_end_io_work(struct work_struct *work)
 	unsigned long		flags;
 	int			ret;
 
+	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
+	if (list_empty(&io->list)) {
+		spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
+		goto free;
+	}
+	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
+
 	if (!mutex_trylock(&inode->i_mutex)) {
 		/*
 		 * Requeue the work instead of waiting so that the work
@@ -170,6 +174,7 @@ static void ext4_end_io_work(struct work_struct *work)
 		list_del_init(&io->list);
 	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
 	mutex_unlock(&inode->i_mutex);
+free:
 	ext4_free_io_end(io);
 }
 
-- 
1.7.0.4

Re: [PATCH] ext4: Check io list state and avoid an unnecessary mutex_lock in ext4_end_io_work.

[Ted Ts'o]

On Wed, Sep 14, 2011 at 04:33:02PM +0800, Tao Ma wrote:
> From: Tao Ma <boyu.mt@taobao.com>
> 
> When we finish the end io work in ext4_flush_completed_IO, we take
> the io work away from the list, but don't free it. Then in the workqueue,
> we can check the list state and then avoid the extra work if it is also
> done. It is good, but we check list state in ext4_end_io_nolock with i_mutex held
> instead of the spin_lock in other places. This is wrong.

Hi Tao,

Thanks for finding and pointing out this bug in ext4_end_io_nolock().
Unfortunately your proposed fix doesn't take into account that there
are other callers of ext4_end_io_nolock() besides ext4_end_io_work(),
and so it's not sufficient to move the test from former function to
the latter.

Attached please find the patch which I am planning to check into the
ext4 tree to address this bug which you have pointed out.

Regards,

						- Ted

commit c0e36d8410bfad4db4edefeb4175f85a5d216c8d
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Sat Oct 29 16:57:19 2011 -0400

    ext4: use spinlock before checking io->list in ext4_io_end_nolock()
    
    In ext4_end_io_nolock(), io->list is checked to see if it is empty
    without taking the ei->i_completed_io_lock spinlock.  This violates
    the locking protocol, and can cause very hard to debug failures.
    
    Also optimize ext4_end_io_work() so that if ext4_end_io_nolock() is
    not going to do any work, don't try getting the i_mutex and possibly
    requeuing the end_io request if the trylock doesn't succeed in grabbing
    the mutex.
    
    Thanks to Tao Ma <boyu.mt@taobao.com> for spotting the error and
    providing an initial fix to address the problem.
    
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
    Signed-off-by: Tao Ma <boyu.mt@taobao.com>

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 92f38ee..0af5607 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -90,21 +90,27 @@ void ext4_free_io_end(ext4_io_end_t *io)
  */
 int ext4_end_io_nolock(ext4_io_end_t *io)
 {
-	struct inode *inode = io->inode;
-	loff_t offset = io->offset;
-	ssize_t size = io->size;
-	wait_queue_head_t *wq;
-	int ret = 0;
+	unsigned long		flags;
+	struct inode		*inode = io->inode;
+	loff_t			offset = io->offset;
+	ssize_t			size = io->size;
+	struct ext4_inode_info	*ei = EXT4_I(inode);
+	wait_queue_head_t	*wq;
+	int			ret;
 
 	ext4_debug("ext4_end_io_nolock: io 0x%p from inode %lu,list->next 0x%p,"
 		   "list->prev 0x%p\n",
 		   io, inode->i_ino, io->list.next, io->list.prev);
 
-	if (list_empty(&io->list))
-		return ret;
+	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
+	if (list_empty(&io->list)) {
+		spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
+		return 0;
+	}
+	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
 
 	if (!(io->flag & EXT4_IO_END_UNWRITTEN))
-		return ret;
+		return 0;
 
 	ret = ext4_convert_unwritten_extents(inode, offset, size);
 	if (ret < 0) {
@@ -142,6 +148,16 @@ static void ext4_end_io_work(struct work_struct *work)
 	unsigned long		flags;
 	int			ret;
 
+	if (!(io->flag & EXT4_IO_END_UNWRITTEN))
+		goto free_io_end;
+
+	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
+	if (list_empty(&io->list)) {
+		spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
+		goto free_io_end;
+	}
+	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
+
 	if (!mutex_trylock(&inode->i_mutex)) {
 		/*
 		 * Requeue the work instead of waiting so that the work
@@ -170,6 +186,7 @@ static void ext4_end_io_work(struct work_struct *work)
 		list_del_init(&io->list);
 	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
 	mutex_unlock(&inode->i_mutex);
+free_io_end:
 	ext4_free_io_end(io);
 }
 

Re: [PATCH] ext4: Check io list state and avoid an unnecessary mutex_lock in ext4_end_io_work.

[Tao Ma]

Hi Ted,
On 10/30/2011 04:57 AM, Ted Ts'o wrote:
> On Wed, Sep 14, 2011 at 04:33:02PM +0800, Tao Ma wrote:
>> From: Tao Ma <boyu.mt@taobao.com>
>>
>> When we finish the end io work in ext4_flush_completed_IO, we take
>> the io work away from the list, but don't free it. Then in the workqueue,
>> we can check the list state and then avoid the extra work if it is also
>> done. It is good, but we check list state in ext4_end_io_nolock with i_mutex held
>> instead of the spin_lock in other places. This is wrong.
> 
> Hi Tao,
> 
> Thanks for finding and pointing out this bug in ext4_end_io_nolock().
> Unfortunately your proposed fix doesn't take into account that there
> are other callers of ext4_end_io_nolock() besides ext4_end_io_work(),
> and so it's not sufficient to move the test from former function to
> the latter.
sorry, but I thought I had considered this case.
There are 2 callers. One is ext4_end_io_work(which has the bug I pointed
out), the other is ext4_flush_complete_IO which has already done the
check before calling ext4_end_io_nolock. And that's the reason why I
move the check from ext4_end_io_nolock to ext4_end_io_work. So for the
ext4_flush_complete_IO case, your new patch will spin_lock twice for the
checking. Do I miss something here?

Thanks
Tao
> 
> Attached please find the patch which I am planning to check into the
> ext4 tree to address this bug which you have pointed out.
> 
> Regards,
> 
> 						- Ted
> 
> commit c0e36d8410bfad4db4edefeb4175f85a5d216c8d
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Sat Oct 29 16:57:19 2011 -0400
> 
>     ext4: use spinlock before checking io->list in ext4_io_end_nolock()
>     
>     In ext4_end_io_nolock(), io->list is checked to see if it is empty
>     without taking the ei->i_completed_io_lock spinlock.  This violates
>     the locking protocol, and can cause very hard to debug failures.
>     
>     Also optimize ext4_end_io_work() so that if ext4_end_io_nolock() is
>     not going to do any work, don't try getting the i_mutex and possibly
>     requeuing the end_io request if the trylock doesn't succeed in grabbing
>     the mutex.
>     
>     Thanks to Tao Ma <boyu.mt@taobao.com> for spotting the error and
>     providing an initial fix to address the problem.
>     
>     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>     Signed-off-by: Tao Ma <boyu.mt@taobao.com>
> 
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 92f38ee..0af5607 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -90,21 +90,27 @@ void ext4_free_io_end(ext4_io_end_t *io)
>   */
>  int ext4_end_io_nolock(ext4_io_end_t *io)
>  {
> -	struct inode *inode = io->inode;
> -	loff_t offset = io->offset;
> -	ssize_t size = io->size;
> -	wait_queue_head_t *wq;
> -	int ret = 0;
> +	unsigned long		flags;
> +	struct inode		*inode = io->inode;
> +	loff_t			offset = io->offset;
> +	ssize_t			size = io->size;
> +	struct ext4_inode_info	*ei = EXT4_I(inode);
> +	wait_queue_head_t	*wq;
> +	int			ret;
>  
>  	ext4_debug("ext4_end_io_nolock: io 0x%p from inode %lu,list->next 0x%p,"
>  		   "list->prev 0x%p\n",
>  		   io, inode->i_ino, io->list.next, io->list.prev);
>  
> -	if (list_empty(&io->list))
> -		return ret;
> +	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
> +	if (list_empty(&io->list)) {
> +		spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
> +		return 0;
> +	}
> +	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
>  
>  	if (!(io->flag & EXT4_IO_END_UNWRITTEN))
> -		return ret;
> +		return 0;
>  
>  	ret = ext4_convert_unwritten_extents(inode, offset, size);
>  	if (ret < 0) {
> @@ -142,6 +148,16 @@ static void ext4_end_io_work(struct work_struct *work)
>  	unsigned long		flags;
>  	int			ret;
>  
> +	if (!(io->flag & EXT4_IO_END_UNWRITTEN))
> +		goto free_io_end;
> +
> +	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
> +	if (list_empty(&io->list)) {
> +		spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
> +		goto free_io_end;
> +	}
> +	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
> +
>  	if (!mutex_trylock(&inode->i_mutex)) {
>  		/*
>  		 * Requeue the work instead of waiting so that the work
> @@ -170,6 +186,7 @@ static void ext4_end_io_work(struct work_struct *work)
>  		list_del_init(&io->list);
>  	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
>  	mutex_unlock(&inode->i_mutex);
> +free_io_end:
>  	ext4_free_io_end(io);
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: Check io list state and avoid an unnecessary mutex_lock in ext4_end_io_work.

[Ted Ts'o]

On Sun, Oct 30, 2011 at 03:50:25PM +0800, Tao Ma wrote:
> sorry, but I thought I had considered this case.
> There are 2 callers. One is ext4_end_io_work(which has the bug I pointed
> out), the other is ext4_flush_complete_IO which has already done the
> check before calling ext4_end_io_nolock. And that's the reason why I
> move the check from ext4_end_io_nolock to ext4_end_io_work. So for the
> ext4_flush_complete_IO case, your new patch will spin_lock twice for the
> checking. Do I miss something here?

Ah, you're right; my mistake.  When I looked closely, though, I found
that ext4_flush_completed_IO() had a call to list_empty() without
taking the spinlock, which would also be problematic.  When I looked
more closely, I found more ways to optimize things, which also close
up a few potential (I think theoretical) race conditions.

Let me know what you think....

					- Ted

[PATCH 1/3] ext4: Use correct locking for ext4_end_io_nolock()

[Theodore Ts'o]

From: Tao Ma <boyu.mt@taobao.com>

We must hold i_completed_io_lock when manipulating anything on the
i_completed_io_list linked list.  This includes io->lock, which we
were checking in ext4_end_io_nolock().

So move this check to ext4_end_io_work().  This also has the bonus of
avoiding extra work if it is already done without needing to take the
mutex.

Signed-off-by: Tao Ma <boyu.mt@taobao.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/fsync.c   |    3 ---
 fs/ext4/page-io.c |   14 +++++++++++---
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index c942924..851ac5b 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -83,9 +83,6 @@ int ext4_flush_completed_IO(struct inode *inode)
 	int ret = 0;
 	int ret2 = 0;
 
-	if (list_empty(&ei->i_completed_io_list))
-		return ret;
-
 	dump_completed_IO(inode);
 	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
 	while (!list_empty(&ei->i_completed_io_list)){
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 92f38ee..aed4096 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -87,6 +87,9 @@ void ext4_free_io_end(ext4_io_end_t *io)
 
 /*
  * check a range of space and convert unwritten extents to written.
+ *
+ * Called with inode->i_mutex; we depend on this when we manipulate
+ * io->flag, since we could otherwise race with ext4_flush_completed_IO()
  */
 int ext4_end_io_nolock(ext4_io_end_t *io)
 {
@@ -100,9 +103,6 @@ int ext4_end_io_nolock(ext4_io_end_t *io)
 		   "list->prev 0x%p\n",
 		   io, inode->i_ino, io->list.next, io->list.prev);
 
-	if (list_empty(&io->list))
-		return ret;
-
 	if (!(io->flag & EXT4_IO_END_UNWRITTEN))
 		return ret;
 
@@ -142,6 +142,13 @@ static void ext4_end_io_work(struct work_struct *work)
 	unsigned long		flags;
 	int			ret;
 
+	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
+	if (list_empty(&io->list)) {
+		spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
+		goto free;
+	}
+	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
+
 	if (!mutex_trylock(&inode->i_mutex)) {
 		/*
 		 * Requeue the work instead of waiting so that the work
@@ -170,6 +177,7 @@ static void ext4_end_io_work(struct work_struct *work)
 		list_del_init(&io->list);
 	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
 	mutex_unlock(&inode->i_mutex);
+free:
 	ext4_free_io_end(io);
 }
 
-- 
1.7.4.1.22.gec8e1.dirty

[PATCH 2/3] ext4: remove unnecessary call to waitqueue_active()

[Theodore Ts'o]

The usage of waitqueue_active() is not necessary, and introduces (I
believe) a hard-to-hit race.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/page-io.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index aed4096..4fa1d70 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -70,7 +70,6 @@ static void put_io_page(struct ext4_io_page *io_page)
 void ext4_free_io_end(ext4_io_end_t *io)
 {
 	int i;
-	wait_queue_head_t *wq;
 
 	BUG_ON(!io);
 	if (io->page)
@@ -78,10 +77,8 @@ void ext4_free_io_end(ext4_io_end_t *io)
 	for (i = 0; i < io->num_io_pages; i++)
 		put_io_page(io->pages[i]);
 	io->num_io_pages = 0;
-	wq = ext4_ioend_wq(io->inode);
-	if (atomic_dec_and_test(&EXT4_I(io->inode)->i_ioend_count) &&
-	    waitqueue_active(wq))
-		wake_up_all(wq);
+	if (atomic_dec_and_test(&EXT4_I(io->inode)->i_ioend_count))
+		wake_up_all(ext4_ioend_wq(io->inode));
 	kmem_cache_free(io_end_cachep, io);
 }
 
@@ -96,7 +93,6 @@ int ext4_end_io_nolock(ext4_io_end_t *io)
 	struct inode *inode = io->inode;
 	loff_t offset = io->offset;
 	ssize_t size = io->size;
-	wait_queue_head_t *wq;
 	int ret = 0;
 
 	ext4_debug("ext4_end_io_nolock: io 0x%p from inode %lu,list->next 0x%p,"
@@ -121,11 +117,8 @@ int ext4_end_io_nolock(ext4_io_end_t *io)
 	if (io->flag & EXT4_IO_END_UNWRITTEN) {
 		io->flag &= ~EXT4_IO_END_UNWRITTEN;
 		/* Wake up anyone waiting on unwritten extent conversion */
-		wq = ext4_ioend_wq(io->inode);
-		if (atomic_dec_and_test(&EXT4_I(inode)->i_aiodio_unwritten) &&
-		    waitqueue_active(wq)) {
-			wake_up_all(wq);
-		}
+		if (atomic_dec_and_test(&EXT4_I(inode)->i_aiodio_unwritten))
+			wake_up_all(ext4_ioend_wq(io->inode));
 	}
 
 	return ret;
-- 
1.7.4.1.22.gec8e1.dirty

[PATCH 3/3] ext4: optimize locking for end_io extent conversion

[Theodore Ts'o]

Now that we are doing the locking correctly, we need to grab the
i_completed_io_lock() twice per end_io.  We can clean this up by
removing the structure from the i_complted_io_list, and use this as
the locking mechanism to prevent ext4_flush_completed_IO() racing
against ext4_end_io_work(), instead of clearing the
EXT4_IO_END_UNWRITTEN in io->flag.

In addition, if the ext4_convert_unwritten_extents() returns an error,
we no longer keep the end_io structure on the linked list.  This
doesn't help, because it tends to lock up the file system and wedges
the system.  That's one way to call attention to the problem, but it
doesn't help the overall robustness of the system.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/fsync.c   |    5 ++---
 fs/ext4/page-io.c |   37 +++++++++++--------------------------
 2 files changed, 13 insertions(+), 29 deletions(-)

diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index 851ac5b..00a2cb7 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -88,6 +88,7 @@ int ext4_flush_completed_IO(struct inode *inode)
 	while (!list_empty(&ei->i_completed_io_list)){
 		io = list_entry(ei->i_completed_io_list.next,
 				ext4_io_end_t, list);
+		list_del_init(&io->list);
 		/*
 		 * Calling ext4_end_io_nolock() to convert completed
 		 * IO to written.
@@ -104,11 +105,9 @@ int ext4_flush_completed_IO(struct inode *inode)
 		 */
 		spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
 		ret = ext4_end_io_nolock(io);
-		spin_lock_irqsave(&ei->i_completed_io_lock, flags);
 		if (ret < 0)
 			ret2 = ret;
-		else
-			list_del_init(&io->list);
+		spin_lock_irqsave(&ei->i_completed_io_lock, flags);
 	}
 	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
 	return (ret2 < 0) ? ret2 : 0;
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 4fa1d70..7bacd27 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -99,28 +99,21 @@ int ext4_end_io_nolock(ext4_io_end_t *io)
 		   "list->prev 0x%p\n",
 		   io, inode->i_ino, io->list.next, io->list.prev);
 
-	if (!(io->flag & EXT4_IO_END_UNWRITTEN))
-		return ret;
-
 	ret = ext4_convert_unwritten_extents(inode, offset, size);
 	if (ret < 0) {
-		printk(KERN_EMERG "%s: failed to convert unwritten "
-			"extents to written extents, error is %d "
-			"io is still on inode %lu aio dio list\n",
-		       __func__, ret, inode->i_ino);
-		return ret;
+		ext4_msg(inode->i_sb, KERN_EMERG,
+			 "failed to convert unwritten extents to written "
+			 "extents -- potential data loss!  "
+			 "(inode %lu, offset %llu, size %d, error %d)",
+			 inode->i_ino, offset, size, ret);
 	}
 
 	if (io->iocb)
 		aio_complete(io->iocb, io->result, 0);
-	/* clear the DIO AIO unwritten flag */
-	if (io->flag & EXT4_IO_END_UNWRITTEN) {
-		io->flag &= ~EXT4_IO_END_UNWRITTEN;
-		/* Wake up anyone waiting on unwritten extent conversion */
-		if (atomic_dec_and_test(&EXT4_I(inode)->i_aiodio_unwritten))
-			wake_up_all(ext4_ioend_wq(io->inode));
-	}
 
+	/* Wake up anyone waiting on unwritten extent conversion */
+	if (atomic_dec_and_test(&EXT4_I(inode)->i_aiodio_unwritten))
+		wake_up_all(ext4_ioend_wq(io->inode));
 	return ret;
 }
 
@@ -133,16 +126,15 @@ static void ext4_end_io_work(struct work_struct *work)
 	struct inode		*inode = io->inode;
 	struct ext4_inode_info	*ei = EXT4_I(inode);
 	unsigned long		flags;
-	int			ret;
 
 	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
 	if (list_empty(&io->list)) {
 		spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
 		goto free;
 	}
-	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
 
 	if (!mutex_trylock(&inode->i_mutex)) {
+		spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
 		/*
 		 * Requeue the work instead of waiting so that the work
 		 * items queued after this can be processed.
@@ -159,16 +151,9 @@ static void ext4_end_io_work(struct work_struct *work)
 		io->flag |= EXT4_IO_END_QUEUED;
 		return;
 	}
-	ret = ext4_end_io_nolock(io);
-	if (ret < 0) {
-		mutex_unlock(&inode->i_mutex);
-		return;
-	}

Re: [PATCH 1/3] ext4: Use correct locking for ext4_end_io_nolock()

[Tao Ma]

On 10/31/2011 11:02 PM, Theodore Ts'o wrote:
> From: Tao Ma <boyu.mt@taobao.com>
> 
> We must hold i_completed_io_lock when manipulating anything on the
> i_completed_io_list linked list.  This includes io->lock, which we
> were checking in ext4_end_io_nolock().
> 
> So move this check to ext4_end_io_work().  This also has the bonus of
> avoiding extra work if it is already done without needing to take the
> mutex.
> 
> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
I am fine with it.

Thanks
Tao
> ---
>  fs/ext4/fsync.c   |    3 ---
>  fs/ext4/page-io.c |   14 +++++++++++---
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
> index c942924..851ac5b 100644
> --- a/fs/ext4/fsync.c
> +++ b/fs/ext4/fsync.c
> @@ -83,9 +83,6 @@ int ext4_flush_completed_IO(struct inode *inode)
>  	int ret = 0;
>  	int ret2 = 0;
>  
> -	if (list_empty(&ei->i_completed_io_list))
> -		return ret;
> -
>  	dump_completed_IO(inode);
>  	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
>  	while (!list_empty(&ei->i_completed_io_list)){
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 92f38ee..aed4096 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -87,6 +87,9 @@ void ext4_free_io_end(ext4_io_end_t *io)
>  
>  /*
>   * check a range of space and convert unwritten extents to written.
> + *
> + * Called with inode->i_mutex; we depend on this when we manipulate
> + * io->flag, since we could otherwise race with ext4_flush_completed_IO()
>   */
>  int ext4_end_io_nolock(ext4_io_end_t *io)
>  {
> @@ -100,9 +103,6 @@ int ext4_end_io_nolock(ext4_io_end_t *io)
>  		   "list->prev 0x%p\n",
>  		   io, inode->i_ino, io->list.next, io->list.prev);
>  
> -	if (list_empty(&io->list))
> -		return ret;
> -
>  	if (!(io->flag & EXT4_IO_END_UNWRITTEN))
>  		return ret;
>  
> @@ -142,6 +142,13 @@ static void ext4_end_io_work(struct work_struct *work)
>  	unsigned long		flags;
>  	int			ret;
>  
> +	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
> +	if (list_empty(&io->list)) {
> +		spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
> +		goto free;
> +	}
> +	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
> +
>  	if (!mutex_trylock(&inode->i_mutex)) {
>  		/*
>  		 * Requeue the work instead of waiting so that the work
> @@ -170,6 +177,7 @@ static void ext4_end_io_work(struct work_struct *work)
>  		list_del_init(&io->list);
>  	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
>  	mutex_unlock(&inode->i_mutex);
> +free:
>  	ext4_free_io_end(io);
>  }
>  

Re: [PATCH 3/3] ext4: optimize locking for end_io extent conversion

[Tao Ma]

Hi Ted,
On Mon, Oct 31, 2011 at 11:02 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> Now that we are doing the locking correctly, we need to grab the
> i_completed_io_lock() twice per end_io.  We can clean this up by
> removing the structure from the i_complted_io_list, and use this as
> the locking mechanism to prevent ext4_flush_completed_IO() racing
> against ext4_end_io_work(), instead of clearing the
> EXT4_IO_END_UNWRITTEN in io->flag.
>
> In addition, if the ext4_convert_unwritten_extents() returns an error,
> we no longer keep the end_io structure on the linked list.  This
> doesn't help, because it tends to lock up the file system and wedges
> the system.  That's one way to call attention to the problem, but it
> doesn't help the overall robustness of the system.
>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/fsync.c   |    5 ++---
>  fs/ext4/page-io.c |   37 +++++++++++--------------------------
>  2 files changed, 13 insertions(+), 29 deletions(-)
>
> diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
> index 851ac5b..00a2cb7 100644
> --- a/fs/ext4/fsync.c
> +++ b/fs/ext4/fsync.c
> @@ -88,6 +88,7 @@ int ext4_flush_completed_IO(struct inode *inode)
>        while (!list_empty(&ei->i_completed_io_list)){
>                io = list_entry(ei->i_completed_io_list.next,
>                                ext4_io_end_t, list);
> +               list_del_init(&io->list);
>                /*
>                 * Calling ext4_end_io_nolock() to convert completed
>                 * IO to written.
> @@ -104,11 +105,9 @@ int ext4_flush_completed_IO(struct inode *inode)
>                 */
>                spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
>                ret = ext4_end_io_nolock(io);
> -               spin_lock_irqsave(&ei->i_completed_io_lock, flags);
>                if (ret < 0)
>                        ret2 = ret;
> -               else
> -                       list_del_init(&io->list);
> +               spin_lock_irqsave(&ei->i_completed_io_lock, flags);
>        }
>        spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
>        return (ret2 < 0) ? ret2 : 0;
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 4fa1d70..7bacd27 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -99,28 +99,21 @@ int ext4_end_io_nolock(ext4_io_end_t *io)
>                   "list->prev 0x%p\n",
>                   io, inode->i_ino, io->list.next, io->list.prev);
>
> -       if (!(io->flag & EXT4_IO_END_UNWRITTEN))
> -               return ret;
> -
>        ret = ext4_convert_unwritten_extents(inode, offset, size);
>        if (ret < 0) {
> -               printk(KERN_EMERG "%s: failed to convert unwritten "
> -                       "extents to written extents, error is %d "
> -                       "io is still on inode %lu aio dio list\n",
> -                      __func__, ret, inode->i_ino);
> -               return ret;
> +               ext4_msg(inode->i_sb, KERN_EMERG,
> +                        "failed to convert unwritten extents to written "
> +                        "extents -- potential data loss!  "
> +                        "(inode %lu, offset %llu, size %d, error %d)",
> +                        inode->i_ino, offset, size, ret);
>        }
>
>        if (io->iocb)
>                aio_complete(io->iocb, io->result, 0);
> -       /* clear the DIO AIO unwritten flag */
> -       if (io->flag & EXT4_IO_END_UNWRITTEN) {
> -               io->flag &= ~EXT4_IO_END_UNWRITTEN;
> -               /* Wake up anyone waiting on unwritten extent conversion */
> -               if (atomic_dec_and_test(&EXT4_I(inode)->i_aiodio_unwritten))
> -                       wake_up_all(ext4_ioend_wq(io->inode));
> -       }
>
> +       /* Wake up anyone waiting on unwritten extent conversion */
> +       if (atomic_dec_and_test(&EXT4_I(inode)->i_aiodio_unwritten))
> +               wake_up_all(ext4_ioend_wq(io->inode));
>        return ret;
>  }
>
> @@ -133,16 +126,15 @@ static void ext4_end_io_work(struct work_struct *work)
>        struct inode            *inode = io->inode;
>        struct ext4_inode_info  *ei = EXT4_I(inode);
>        unsigned long           flags;
> -       int                     ret;
>
>        spin_lock_irqsave(&ei->i_completed_io_lock, flags);
>        if (list_empty(&io->list)) {
>                spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
>                goto free;
>        }
> -       spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
>
>        if (!mutex_trylock(&inode->i_mutex)) {
So here, we spin_lock first and then mutex_lock.
But in  ext4_flush_completed_IO, we mutex_lock first and
then spin_lock. Will lockdep complain about it?
Other than that, the patch looks good to me.

Thanks
Tao
> +               spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
>                /*
>                 * Requeue the work instead of waiting so that the work
>                 * items queued after this can be processed.
> @@ -159,16 +151,9 @@ static void ext4_end_io_work(struct work_struct *work)
>                io->flag |= EXT4_IO_END_QUEUED;
>                return;
>        }
> -       ret = ext4_end_io_nolock(io);
> -       if (ret < 0) {
> -               mutex_unlock(&inode->i_mutex);
> -               return;
> -       }
> -
> -       spin_lock_irqsave(&ei->i_completed_io_lock, flags);
> -       if (!list_empty(&io->list))
> -               list_del_init(&io->list);
> +       list_del_init(&io->list);
>        spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
> +       (void) ext4_end_io_nolock(io);
>        mutex_unlock(&inode->i_mutex);
>  free:
>        ext4_free_io_end(io);
> --
> 1.7.4.1.22.gec8e1.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ext4: optimize locking for end_io extent conversion

[Theodore Tso]

On Oct 31, 2011, at 10:51 PM, Tao Ma wrote:

>>        if (!mutex_trylock(&inode->i_mutex)) {
> So here, we spin_lock first and then mutex_lock.
> But in  ext4_flush_completed_IO, we mutex_lock first and
> then spin_lock. Will lockdep complain about it?
> Other than that, the patch looks good to me.

It won't complain because it's a mutex_trylock(); so by definition it can't cause a deadlock.

-- Ted