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 (Артём Битюцкий)
On Sun, 21 Oct 2007 23:19:41 +0300 Artem Bityutskiy <[email protected]> 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.
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 (Артём Битюцкий)
On Mon, 22 Oct 2007 11:52:33 +0300 Artem Bityutskiy <[email protected]> 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);
}
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 (Артём Битюцкий)
On Mon, 22 Oct 2007 12:38:48 +0300 Artem Bityutskiy <[email protected]> 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...
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 (Артём Битюцкий)