On Fri 06-05-11 11:08:35, Wu Fengguang wrote:
> Pass struct wb_writeback_work all the way down to writeback_sb_inodes(),
> and initialize the struct writeback_control there.
>
> struct writeback_control is basically designed to control writeback of a
> single file, but we keep abuse it for writing multiple files in
> writeback_sb_inodes() and its callers.
>
> It immediately clean things up, e.g. suddenly wbc.nr_to_write vs
> work->nr_pages starts to make sense, and instead of saving and restoring
> pages_skipped in writeback_sb_inodes it can always start with a clean
> zero value.
>
> It also makes a neat IO pattern change: large dirty files are now
> written in the full 4MB writeback chunk size, rather than whatever
> remained quota in wbc->nr_to_write.
>
> Proposed-by: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Signed-off-by: Wu Fengguang <fengguang.wu@xxxxxxxxx>
...
> @@ -543,34 +588,40 @@ static int writeback_sb_inodes(struct su
> requeue_io(inode, wb);
> continue;
> }
> -
> __iget(inode);
> + write_chunk = writeback_chunk_size(work);
> + wbc.nr_to_write = write_chunk;
> + wbc.pages_skipped = 0;
> +
> + writeback_single_inode(inode, wb, &wbc);
>
> - pages_skipped = wbc->pages_skipped;
> - writeback_single_inode(inode, wb, wbc);
> - if (wbc->pages_skipped != pages_skipped) {
> + work->nr_pages -= write_chunk - wbc.nr_to_write;
> + wrote += write_chunk - wbc.nr_to_write;
> + if (wbc.pages_skipped) {
> /*
> * writeback is not making progress due to locked
> * buffers. Skip this inode for now.
> */
> redirty_tail(inode, wb);
> - }
> + } else if (!(inode->i_state & I_DIRTY))
> + wrote++;
> spin_unlock(&inode->i_lock);
> spin_unlock(&wb->list_lock);
> iput(inode);
> cond_resched();
> spin_lock(&wb->list_lock);
> - if (wbc->nr_to_write <= 0)
> - return 1;
> + if (wrote >= MAX_WRITEBACK_PAGES)
> + break;
This definitely deserves a comment (as well as a similar check in
__writeback_inodes_wb()). I guess you bail out here so that we perform the
background limit check and livelocking of for_kupdate/for_background check
often enough. I'm undecided whether it's good to bail out like this. It's
not necessary in some cases (like WB_SYNC_ALL or for_sync writeback) but
OTOH moving the necessary checks here does not look ideal either...
> void writeback_inodes_wb(struct bdi_writeback *wb,
> - struct writeback_control *wbc)
> + struct writeback_control *wbc)
> {
> + struct wb_writeback_work work = {
> + .nr_pages = wbc->nr_to_write,
> + .sync_mode = wbc->sync_mode,
> + .range_cyclic = wbc->range_cyclic,
> + };
> +
> spin_lock(&wb->list_lock);
> if (list_empty(&wb->b_io))
> - queue_io(wb, wbc->older_than_this);
> - __writeback_inodes_wb(wb, wbc);
> + queue_io(wb, NULL);
> + __writeback_inodes_wb(wb, &work);
> spin_unlock(&wb->list_lock);
> -}
Hmm, maybe we should just pass in number of pages (similarly as in
writeback_inodes_sb_nr())? It would look like a cleaner interface than
passing whole writeback_control and then ignoring parts of it.
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
