* [PATCH] ext4: Check io list state and avoid an unnecessary mutex_lock in ext4_end_io_work. @ 2011-09-14 8:33 Tao Ma 2011-10-29 20:57 ` Ted Ts'o 0 siblings, 1 reply; 10+ messages in thread From: Tao Ma @ 2011-09-14 8:33 UTC (permalink / raw) To: linux-ext4; +Cc: Theodore Ts'o 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] ext4: Check io list state and avoid an unnecessary mutex_lock in ext4_end_io_work. 2011-09-14 8:33 [PATCH] ext4: Check io list state and avoid an unnecessary mutex_lock in ext4_end_io_work Tao Ma @ 2011-10-29 20:57 ` Ted Ts'o 2011-10-30 7:50 ` Tao Ma 0 siblings, 1 reply; 10+ messages in thread From: Ted Ts'o @ 2011-10-29 20:57 UTC (permalink / raw) To: Tao Ma; +Cc: linux-ext4 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); } ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] ext4: Check io list state and avoid an unnecessary mutex_lock in ext4_end_io_work. 2011-10-29 20:57 ` Ted Ts'o @ 2011-10-30 7:50 ` Tao Ma 2011-10-31 15:02 ` Ted Ts'o 0 siblings, 1 reply; 10+ messages in thread From: Tao Ma @ 2011-10-30 7:50 UTC (permalink / raw) To: Ted Ts'o; +Cc: linux-ext4 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ext4: Check io list state and avoid an unnecessary mutex_lock in ext4_end_io_work. 2011-10-30 7:50 ` Tao Ma @ 2011-10-31 15:02 ` Ted Ts'o 2011-10-31 15:02 ` [PATCH 1/3] ext4: Use correct locking for ext4_end_io_nolock() Theodore Ts'o 0 siblings, 1 reply; 10+ messages in thread From: Ted Ts'o @ 2011-10-31 15:02 UTC (permalink / raw) To: Tao Ma; +Cc: linux-ext4 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] ext4: Use correct locking for ext4_end_io_nolock() 2011-10-31 15:02 ` Ted Ts'o @ 2011-10-31 15:02 ` Theodore Ts'o 2011-10-31 15:02 ` [PATCH 2/3] ext4: remove unnecessary call to waitqueue_active() Theodore Ts'o ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Theodore Ts'o @ 2011-10-31 15:02 UTC (permalink / raw) To: Ext4 Developers List; +Cc: Tao Ma, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] ext4: remove unnecessary call to waitqueue_active() 2011-10-31 15:02 ` [PATCH 1/3] ext4: Use correct locking for ext4_end_io_nolock() Theodore Ts'o @ 2011-10-31 15:02 ` Theodore Ts'o 2011-10-31 15:02 ` [PATCH 3/3] ext4: optimize locking for end_io extent conversion Theodore Ts'o 2011-10-31 15:28 ` [PATCH 1/3] ext4: Use correct locking for ext4_end_io_nolock() Tao Ma 2 siblings, 0 replies; 10+ messages in thread From: Theodore Ts'o @ 2011-10-31 15:02 UTC (permalink / raw) To: Ext4 Developers List; +Cc: 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] ext4: optimize locking for end_io extent conversion 2011-10-31 15:02 ` [PATCH 1/3] ext4: Use correct locking for ext4_end_io_nolock() Theodore Ts'o 2011-10-31 15:02 ` [PATCH 2/3] ext4: remove unnecessary call to waitqueue_active() Theodore Ts'o @ 2011-10-31 15:02 ` Theodore Ts'o 2011-11-01 2:51 ` Tao Ma 2011-10-31 15:28 ` [PATCH 1/3] ext4: Use correct locking for ext4_end_io_nolock() Tao Ma 2 siblings, 1 reply; 10+ messages in thread From: Theodore Ts'o @ 2011-10-31 15:02 UTC (permalink / raw) To: Ext4 Developers List; +Cc: 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; - } ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] ext4: optimize locking for end_io extent conversion 2011-10-31 15:02 ` [PATCH 3/3] ext4: optimize locking for end_io extent conversion Theodore Ts'o @ 2011-11-01 2:51 ` Tao Ma 2011-11-01 10:30 ` Theodore Tso 0 siblings, 1 reply; 10+ messages in thread From: Tao Ma @ 2011-11-01 2:51 UTC (permalink / raw) To: Theodore Ts'o; +Cc: Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] ext4: optimize locking for end_io extent conversion 2011-11-01 2:51 ` Tao Ma @ 2011-11-01 10:30 ` Theodore Tso 0 siblings, 0 replies; 10+ messages in thread From: Theodore Tso @ 2011-11-01 10:30 UTC (permalink / raw) To: Tao Ma; +Cc: Theodore Tso, Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ext4: Use correct locking for ext4_end_io_nolock() 2011-10-31 15:02 ` [PATCH 1/3] ext4: Use correct locking for ext4_end_io_nolock() Theodore Ts'o 2011-10-31 15:02 ` [PATCH 2/3] ext4: remove unnecessary call to waitqueue_active() Theodore Ts'o 2011-10-31 15:02 ` [PATCH 3/3] ext4: optimize locking for end_io extent conversion Theodore Ts'o @ 2011-10-31 15:28 ` Tao Ma 2 siblings, 0 replies; 10+ messages in thread From: Tao Ma @ 2011-10-31 15:28 UTC (permalink / raw) To: Theodore Ts'o; +Cc: Ext4 Developers List, 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); > } > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-11-01 10:30 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-09-14 8:33 [PATCH] ext4: Check io list state and avoid an unnecessary mutex_lock in ext4_end_io_work Tao Ma 2011-10-29 20:57 ` Ted Ts'o 2011-10-30 7:50 ` Tao Ma 2011-10-31 15:02 ` Ted Ts'o 2011-10-31 15:02 ` [PATCH 1/3] ext4: Use correct locking for ext4_end_io_nolock() Theodore Ts'o 2011-10-31 15:02 ` [PATCH 2/3] ext4: remove unnecessary call to waitqueue_active() Theodore Ts'o 2011-10-31 15:02 ` [PATCH 3/3] ext4: optimize locking for end_io extent conversion Theodore Ts'o 2011-11-01 2:51 ` Tao Ma 2011-11-01 10:30 ` Theodore Tso 2011-10-31 15:28 ` [PATCH 1/3] ext4: Use correct locking for ext4_end_io_nolock() Tao Ma
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).