[PATCH 1/1] writeback fix bdi thread race in mark_inode_dirty

[PATCH 1/1] writeback fix bdi thread race in mark_inode_dirty

[Srinivas Eeda]

In __mark_inode_dirty, a process checks !wb_has_dirty_io outside of list_lock
spinlock. This could cause a race, where process sees that list has dirty io
and decides not wake up bdi thread and waits for spinlock to add to dirty list.
Right at this time bdi_writeback_workfn finished write-back on last inode.
It sees the list is empty and ends. Process could now get the spinlock and
add inode to dirty list and doesn't wakeup bdi thread. Future calls to
__mark_inode_dirty also do not wake up the thread because list is not empty
any more.

Fix is to get wb.list_lock spinlock before checking the dirty list

Signed-off-by: Srinivas Eeda <srinivas.eeda@oracle.com>
---
 fs/fs-writeback.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 68851ff..a5abc6c 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1173,6 +1173,9 @@ void __mark_inode_dirty(struct inode *inode, int flags)
 			bool wakeup_bdi = false;
 			bdi = inode_to_bdi(inode);
 
+			spin_unlock(&inode->i_lock);
+			spin_lock(&bdi->wb.list_lock);
+
 			if (bdi_cap_writeback_dirty(bdi)) {
 				WARN(!test_bit(BDI_registered, &bdi->state),
 				     "bdi-%s not registered\n", bdi->name);
@@ -1187,8 +1190,6 @@ void __mark_inode_dirty(struct inode *inode, int flags)
 					wakeup_bdi = true;
 			}
 
-			spin_unlock(&inode->i_lock);
-			spin_lock(&bdi->wb.list_lock);
 			inode->dirtied_when = jiffies;
 			list_move(&inode->i_wb_list, &bdi->wb.b_dirty);
 			spin_unlock(&bdi->wb.list_lock);
-- 
1.5.6.5

Re: [PATCH 1/1] writeback fix bdi thread race in mark_inode_dirty

[Jan Kara]

On Fri 19-07-13 11:05:40, Srinivas Eeda wrote:
> In __mark_inode_dirty, a process checks !wb_has_dirty_io outside of list_lock
> spinlock. This could cause a race, where process sees that list has dirty io
> and decides not wake up bdi thread and waits for spinlock to add to dirty list.
> Right at this time bdi_writeback_workfn finished write-back on last inode.
> It sees the list is empty and ends. Process could now get the spinlock and
> add inode to dirty list and doesn't wakeup bdi thread. Future calls to
> __mark_inode_dirty also do not wake up the thread because list is not empty
> any more.
> 
> Fix is to get wb.list_lock spinlock before checking the dirty list
> 
> Signed-off-by: Srinivas Eeda <srinivas.eeda@oracle.com>
  Good catch! The patch looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  fs/fs-writeback.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 68851ff..a5abc6c 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -1173,6 +1173,9 @@ void __mark_inode_dirty(struct inode *inode, int flags)
>  			bool wakeup_bdi = false;
>  			bdi = inode_to_bdi(inode);
>  
> +			spin_unlock(&inode->i_lock);
> +			spin_lock(&bdi->wb.list_lock);
> +
>  			if (bdi_cap_writeback_dirty(bdi)) {
>  				WARN(!test_bit(BDI_registered, &bdi->state),
>  				     "bdi-%s not registered\n", bdi->name);
> @@ -1187,8 +1190,6 @@ void __mark_inode_dirty(struct inode *inode, int flags)
>  					wakeup_bdi = true;
>  			}
>  
> -			spin_unlock(&inode->i_lock);
> -			spin_lock(&bdi->wb.list_lock);
>  			inode->dirtied_when = jiffies;
>  			list_move(&inode->i_wb_list, &bdi->wb.b_dirty);
>  			spin_unlock(&bdi->wb.list_lock);
> -- 
> 1.5.6.5
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/1] writeback fix bdi thread race in mark_inode_dirty

[Jens Axboe]

On Fri, Jul 19 2013, Srinivas Eeda wrote:
> In __mark_inode_dirty, a process checks !wb_has_dirty_io outside of list_lock
> spinlock. This could cause a race, where process sees that list has dirty io
> and decides not wake up bdi thread and waits for spinlock to add to dirty list.
> Right at this time bdi_writeback_workfn finished write-back on last inode.
> It sees the list is empty and ends. Process could now get the spinlock and
> add inode to dirty list and doesn't wakeup bdi thread. Future calls to
> __mark_inode_dirty also do not wake up the thread because list is not empty
> any more.
> 
> Fix is to get wb.list_lock spinlock before checking the dirty list

With Jan's ack, lets add this for the current cycle. Should go into
stable as well, imho.

-- 
Jens Axboe

Re: [PATCH 1/1] writeback fix bdi thread race in mark_inode_dirty

[Srinivas Eeda]

Hi Jens,

Thanks!

I am new to submitting patches to linux-fsdevel. Is there a mailing list 
I should submit this patch or someone will pick this patch for mainline 
and stable branch?

Thanks,
--Srini

On 07/22/2013 08:41 AM, Jens Axboe wrote:
> On Fri, Jul 19 2013, Srinivas Eeda wrote:
>> In __mark_inode_dirty, a process checks !wb_has_dirty_io outside of list_lock
>> spinlock. This could cause a race, where process sees that list has dirty io
>> and decides not wake up bdi thread and waits for spinlock to add to dirty list.
>> Right at this time bdi_writeback_workfn finished write-back on last inode.
>> It sees the list is empty and ends. Process could now get the spinlock and
>> add inode to dirty list and doesn't wakeup bdi thread. Future calls to
>> __mark_inode_dirty also do not wake up the thread because list is not empty
>> any more.
>>
>> Fix is to get wb.list_lock spinlock before checking the dirty list
> With Jan's ack, lets add this for the current cycle. Should go into
> stable as well, imho.
>

Re: [PATCH 1/1] writeback fix bdi thread race in mark_inode_dirty

[Jens Axboe]

On Mon, Jul 22 2013, Srinivas Eeda wrote:
> Hi Jens,
> 
> Thanks!
> 
> I am new to submitting patches to linux-fsdevel. Is there a mailing list I
> should submit this patch or someone will pick this patch for mainline and
> stable branch?

No that's fine. What is needed is a stable@kernel.org CC, but that can
easily be added after the fact.

-- 
Jens Axboe