On Feb 18, 2014 4:55 PM, "Tejun Heo" [off-list ref] wrote:

Hello,

On Fri, Feb 14, 2014 at 08:12:17PM -0800, Derek Basehore wrote:

quoted

bdi_wakeup_thread_delayed used the mod_delayed_work function to

schedule work

quoted

to writeback dirty inodes. The problem with this is that it can delay

work that

quoted

is scheduled for immediate execution, such as the work from

sync_inodes_sb.

quoted

This can happen since mod_delayed_work can now steal work from a

work_queue.

quoted

This fixes the problem by using queue_delayed_work instead. This is a
regression from the move to the bdi workqueue design.

The reason that this causes a problem is that laptop-mode will change

the

quoted

delay, dirty_writeback_centisecs, to 60000 (10 minutes) by default. In

the case

quoted

that bdi_wakeup_thread_delayed races with sync_inodes_sb, sync will be

stopped

quoted

for 10 minutes and trigger a hung task. Even if

dirty_writeback_centisecs is

quoted

not long enough to cause a hung task, we still don't want to delay sync

for

quoted

that long.

Oops.

quoted

For the same reason, this also changes bdi_writeback_workfn to

immediately

quoted

queue the work again in the case that the work_list is not empty. The

same

quoted

problem can happen if the sync work is run on the rescue worker.

Signed-off-by: Derek Basehore <redacted>
---
 fs/fs-writeback.c | 5 +++--
 mm/backing-dev.c  | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index e0259a1..95b7b8c 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c

@@ -1047,8 +1047,9 @@ void bdi_writeback_workfn(struct work_struct

*work)

quoted

              trace_writeback_pages_written(pages_written);
      }

-     if (!list_empty(&bdi->work_list) ||
-         (wb_has_dirty_io(wb) && dirty_writeback_interval))
+     if (!list_empty(&bdi->work_list))
+             mod_delayed_work(bdi_wq, &wb->dwork, 0);
+     else if (wb_has_dirty_io(wb) && dirty_writeback_interval)
              queue_delayed_work(bdi_wq, &wb->dwork,
                      msecs_to_jiffies(dirty_writeback_interval * 10));

Can you please add some comments explaining why the specific variants
are being used here?

Will do this weekend. I'm away from my computer until then.

quoted

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index ce682f7..3fde024 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c

@@ -294,7 +294,7 @@ void bdi_wakeup_thread_delayed(struct

backing_dev_info *bdi)

quoted

      unsigned long timeout;

      timeout = msecs_to_jiffies(dirty_writeback_interval * 10);
-     mod_delayed_work(bdi_wq, &bdi->wb.dwork, timeout);
+     queue_delayed_work(bdi_wq, &bdi->wb.dwork, timeout);

and here?

Hmmm.... but doesn't this create an opposite problem?  Now a flush
queued for an earlier time may be overridden by something scheduled
later, no?

Thanks.

--
tejun