forcing write-back from FS - again

forcing write-back from FS - again

[Artem Bityutskiy]

Hi Andrew,

some time ago we were talking about doing write-back from inside a file-system 
(http://marc.info/?l=linux-kernel&m=119097117713616&w=2). You said that I'm not 
the only person who needs this, because the same thing is needed for delayed 
allocation.

The problem is that if we initiate write-back from prepare_write() and we are 
having a dirty page lock, we deadlock in write_cache_pages() which tries to 
lock the same page.

You suggested to enhance struct writeback_control and put page that should be 
skipped.

I tried something like

diff --git a/include/linux/writeback.h b/include/linux/writeback.h
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -61,6 +61,7 @@ struct writeback_control {
         unsigned for_reclaim:1;         /* Invoked from the page allocator */
         unsigned for_writepages:1;      /* This is a writepages() call */
         unsigned range_cyclic:1;        /* range_start is cyclic */
+       struct page *skip_pg;           /* do not write this page back */

         void *fs_private;               /* For use by ->writepages() */
  };

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -641,6 +641,9 @@ retry:
                 for (i = 0; i < nr_pages; i++) {
                         struct page *page = pvec.pages[i];

+                       if (unlikely(page == wbc->skip_pg))
+                               continue;
+
                         /*
                          * At this point we hold neither mapping->tree_lock nor
                          * lock on the page itself: the page may be truncated

but it does not dot actually work, because if we have two processes forcing 
write-back from write_page(), they will mutually deadlock (A waits in 
write_cache_pages() on a page B has locked, B waits on inode or page A has locked).

So this way is not ok, do you have any other ideas?

We could mark page clean temporarily before doing write-back, and mark it dirty 
again, but this seems to be inefficient (although I'm not sure, need to dig 
these functions deeper, but they _seem_ to traverse the radix tree and change 
tags, so marking one page dirty may need to change many tags, but again, I did 
not really dig tis yet).

I'd appreciate any suggestions. Thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: forcing write-back from FS - again

[Andrew Morton]

On Sun, 21 Oct 2007 23:19:41 +0300 Artem Bityutskiy <Artem.Bityutskiy@nokia.com> wrote:

> Hi Andrew,
> 
> some time ago we were talking about doing write-back from inside a file-system 
> (http://marc.info/?l=linux-kernel&m=119097117713616&w=2). You said that I'm not 
> the only person who needs this, because the same thing is needed for delayed 
> allocation.
> 
> The problem is that if we initiate write-back from prepare_write() and we are 
> having a dirty page lock, we deadlock in write_cache_pages() which tries to 
> lock the same page.
> 
> You suggested to enhance struct writeback_control and put page that should be 
> skipped.
> 
> ...
>
> but it does not dot actually work, because if we have two processes forcing 
> write-back from write_page(), they will mutually deadlock (A waits in 
> write_cache_pages() on a page B has locked, B waits on inode or page A has locked).

Yeah, I was just thinking that as I read this ;)
 
> So this way is not ok, do you have any other ideas?
> 
> We could mark page clean temporarily before doing write-back, and mark it dirty 
> again, but this seems to be inefficient (although I'm not sure, need to dig 
> these functions deeper, but they _seem_ to traverse the radix tree and change 
> tags, so marking one page dirty may need to change many tags, but again, I did 
> not really dig tis yet).
> 
> I'd appreciate any suggestions. Thanks!

We could just skip locked pages altogether in writeback.  Perhaps in
WB_SYNC_NONE mode, or perhaps add a new flag in writeback_control to select
this behaviour.

It _should_ be the case that the number of locked-and-dirty pages which
writeback encounters is very small, so skipping locked pages during
writeback-for-memory-flushing won't have any significant effect.  The first
step should be to add a new /proc/vmstat field to count these pages and
then do broad testing (especially on blocksize<pagesize filesystems) to
confirm the theory.

We'll still need to synchronously lock the page in
writeback-for-data-integrity mode though.

Re: forcing write-back from FS - again

[Artem Bityutskiy]

Andrew Morton wrote:
>> but it does not dot actually work, because if we have two processes forcing 
>> write-back from write_page(), they will mutually deadlock (A waits in 
>> write_cache_pages() on a page B has locked, B waits on inode or page A has locked).
> 
> Yeah, I was just thinking that as I read this ;)
>  
>> So this way is not ok, do you have any other ideas?
>>
>> We could mark page clean temporarily before doing write-back, and mark it dirty 
>> again, but this seems to be inefficient (although I'm not sure, need to dig 
>> these functions deeper, but they _seem_ to traverse the radix tree and change 
>> tags, so marking one page dirty may need to change many tags, but again, I did 
>> not really dig tis yet).
> 
> We could just skip locked pages altogether in writeback.  Perhaps in
> WB_SYNC_NONE mode, or perhaps add a new flag in writeback_control to select
> this behaviour.

Yeah, certanly not WB_SYNC_ALL, because this is a deadlocky - the process which 
forces write-back from the ->prepare_write() is having page X locked, pdflush 
may have some inode A locked and sleep on page X, while the FS would sleep on 
inode A.

> It _should_ be the case that the number of locked-and-dirty pages which
> writeback encounters is very small, so skipping locked pages during
> writeback-for-memory-flushing won't have any significant effect.  The first
> step should be to add a new /proc/vmstat field to count these pages and
> then do broad testing (especially on blocksize<pagesize filesystems) to
> confirm the theory.
> 
> We'll still need to synchronously lock the page in
> writeback-for-data-integrity mode though.

Thanks for suggestion. It sounds as a separate big job to enhance existing 
WB_SYNC_NONE. I've just introduced new WB mode, and it seems to work fine:

diff --git a/include/linux/writeback.h b/include/linux/writeback.h
@@ -28,6 +28,7 @@ static inline int task_is_pdflush(struct task_struct *task)
   */
  enum writeback_sync_modes {
         WB_SYNC_NONE,   /* Don't wait on anything */
+       WB_SYNC_NONE_PG,/* Don't wait on anything, don't touch locked pages */
         WB_SYNC_ALL,    /* Wait on every mapping */
         WB_SYNC_HOLD,   /* Hold the inode on sb_dirty for sys_sync() */
  };
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
@@ -641,6 +641,10 @@ retry:
                 for (i = 0; i < nr_pages; i++) {
                         struct page *page = pvec.pages[i];

+                       if (wbc->sync_mode == WB_SYNC_NONE_PG &&
+                           PageLocked(page))
+                               continue;
+

My only concern is - what if the page we skipped because of WB_SYNC_NONE_PG 
will somehow loose its dirty TAG and will never be written-back? But it is 
because of my poor knowledge of Linux MM internals. Could you please comment on 
this?

Thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: forcing write-back from FS - again

[Andrew Morton]

On Mon, 22 Oct 2007 11:52:33 +0300 Artem Bityutskiy <dedekind@yandex.ru> wrote:

> > It _should_ be the case that the number of locked-and-dirty pages which
> > writeback encounters is very small, so skipping locked pages during
> > writeback-for-memory-flushing won't have any significant effect.  The first
> > step should be to add a new /proc/vmstat field to count these pages and
> > then do broad testing (especially on blocksize<pagesize filesystems) to
> > confirm the theory.
> > 
> > We'll still need to synchronously lock the page in
> > writeback-for-data-integrity mode though.
> 
> Thanks for suggestion. It sounds as a separate big job to enhance existing 
> WB_SYNC_NONE. I've just introduced new WB mode, and it seems to work fine:
> 
> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> @@ -28,6 +28,7 @@ static inline int task_is_pdflush(struct task_struct *task)
>    */
>   enum writeback_sync_modes {
>          WB_SYNC_NONE,   /* Don't wait on anything */
> +       WB_SYNC_NONE_PG,/* Don't wait on anything, don't touch locked pages */
>          WB_SYNC_ALL,    /* Wait on every mapping */
>          WB_SYNC_HOLD,   /* Hold the inode on sb_dirty for sys_sync() */
>   };

It would be simpler/safer/saner to add a new bitflag to writeback_control
and use that directly.  The WB_SYNC_foo flags are a holdover from an
earlier time and really should be made to go away, in favour of directly
setting up an appropriate writeback_control.


> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> @@ -641,6 +641,10 @@ retry:
>                  for (i = 0; i < nr_pages; i++) {
>                          struct page *page = pvec.pages[i];
> 
> +                       if (wbc->sync_mode == WB_SYNC_NONE_PG &&
> +                           PageLocked(page))
> +                               continue;
> +
> 
> My only concern is - what if the page we skipped because of WB_SYNC_NONE_PG 
> will somehow loose its dirty TAG and will never be written-back? But it is 
> because of my poor knowledge of Linux MM internals. Could you please comment on 
> this?

Well it might lose its dirty tag, if the thread which has a lock on the
page is about to write it out or truncate it.  But that shouldn't concern
you here.

The code you have there looks racy: if someone else locks the page in that
little window after the PageLocked() test we'll still block in lock_page().
 That's unlikely to happen in your application (apart from a remaining
ab/ba scenario) but we should make it robust:

	if (wbc->skip_locked_pages) {
		if (TestSetPageLocked(page))
			continue;
	} else {
		lock_page(page);
	}

Re: forcing write-back from FS - again

[Artem Bityutskiy]

Andrew Morton wrote:
>> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
>> @@ -28,6 +28,7 @@ static inline int task_is_pdflush(struct task_struct *task)
>>    */
>>   enum writeback_sync_modes {
>>          WB_SYNC_NONE,   /* Don't wait on anything */
>> +       WB_SYNC_NONE_PG,/* Don't wait on anything, don't touch locked pages */
>>          WB_SYNC_ALL,    /* Wait on every mapping */
>>          WB_SYNC_HOLD,   /* Hold the inode on sb_dirty for sys_sync() */
>>   };
> 
> It would be simpler/safer/saner to add a new bitflag to writeback_control
> and use that directly.  The WB_SYNC_foo flags are a holdover from an
> earlier time and really should be made to go away, in favour of directly
> setting up an appropriate writeback_control.

You mean something like (wbc->flags & WB_SYNC_NONE) etc? But below you used 
wbc->skip_locked_pages, I'm confused.

> The code you have there looks racy: if someone else locks the page in that
> little window after the PageLocked() test we'll still block in lock_page().
>  That's unlikely to happen in your application (apart from a remaining
> ab/ba scenario) but we should make it robust:
> 
> 	if (wbc->skip_locked_pages) {
> 		if (TestSetPageLocked(page))
> 			continue;
> 	} else {
> 		lock_page(page);
> 	}

Yeah, thanks for the pointer! Thank you for your help on the issue!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: forcing write-back from FS - again

[Andrew Morton]

On Mon, 22 Oct 2007 12:38:48 +0300 Artem Bityutskiy <dedekind@yandex.ru> wrote:

> Andrew Morton wrote:
> >> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> >> @@ -28,6 +28,7 @@ static inline int task_is_pdflush(struct task_struct *task)
> >>    */
> >>   enum writeback_sync_modes {
> >>          WB_SYNC_NONE,   /* Don't wait on anything */
> >> +       WB_SYNC_NONE_PG,/* Don't wait on anything, don't touch locked pages */
> >>          WB_SYNC_ALL,    /* Wait on every mapping */
> >>          WB_SYNC_HOLD,   /* Hold the inode on sb_dirty for sys_sync() */
> >>   };
> > 
> > It would be simpler/safer/saner to add a new bitflag to writeback_control
> > and use that directly.  The WB_SYNC_foo flags are a holdover from an
> > earlier time and really should be made to go away, in favour of directly
> > setting up an appropriate writeback_control.
> 
> You mean something like (wbc->flags & WB_SYNC_NONE) etc? But below you used 
> wbc->skip_locked_pages, I'm confused.

take a look at struct writeback_control:

        unsigned nonblocking:1;         /* Don't get stuck on request queues */
        unsigned encountered_congestion:1; /* An output: a queue is full */
        unsigned for_kupdate:1;         /* A kupdate writeback */
        unsigned for_reclaim:1;         /* Invoked from the page allocator */
        unsigned for_writepages:1;      /* This is a writepages() call */
        unsigned range_cyclic:1;        /* range_start is cyclic */
        unsigned more_io:1;             /* more io to be dispatched */

Add another one...

Re: forcing write-back from FS - again

[Artem Bityutskiy]

Andrew Morton wrote:
> take a look at struct writeback_control:
> 
>         unsigned nonblocking:1;         /* Don't get stuck on request queues */
>         unsigned encountered_congestion:1; /* An output: a queue is full */
>         unsigned for_kupdate:1;         /* A kupdate writeback */
>         unsigned for_reclaim:1;         /* Invoked from the page allocator */
>         unsigned for_writepages:1;      /* This is a writepages() call */
>         unsigned range_cyclic:1;        /* range_start is cyclic */
>         unsigned more_io:1;             /* more io to be dispatched */
> 
> Add another one...

Ups, OK! Thanks again!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)