Re: [PATCH] ext4: fix checking on nr_to_write

Jan Kara
2013-10-15T12:34:39Z

On Tue 15-10-13 19:15:56, Ming Lei wrote:
> > This won't quite work because if the page is fully mapped
> > mpage_process_page_bufs() will immediately submit the page and decrease
> > nr_to_write. So now you would end up writing less than you were asked for
> > in some cases.
>
> Yes, your are right, so how about below?
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 32c04ab..3cf7abb 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2295,6 +2295,7 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
> struct address_space *mapping = mpd->inode->i_mapping;
> struct pagevec pvec;
> unsigned int nr_pages;
> + int left = mpd->wbc->nr_to_write;
'long' please. Otherwise the patch looks fine. Thanks!

Honza

> pgoff_t index = mpd->first_page;
> pgoff_t end = mpd->last_page;
> int tag;
> @@ -2330,6 +2331,17 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
> if (page->index > end)
> goto out;
>
> + /*
> + * Accumulated enough dirty pages? This doesn't apply
> + * to WB_SYNC_ALL mode. For integrity sync we have to
> + * keep going because someone may be concurrently
> + * dirtying pages, and we might have synced a lot of
> + * newly appeared dirty pages, but have not synced all
> + * of the old dirty pages.
> + */
> + if (mpd->wbc->sync_mode == WB_SYNC_NONE && left <= 0)
> + goto out;
> +
> /* If we can't merge this page, we are done. */
> if (mpd->map.m_len > 0 && mpd->next_page != page->index)
> goto out;
> @@ -2364,19 +2376,7 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
> if (err <= 0)
> goto out;
> err = 0;
> -
> - /*
> - * Accumulated enough dirty pages? This doesn't apply
> - * to WB_SYNC_ALL mode. For integrity sync we have to
> - * keep going because someone may be concurrently
> - * dirtying pages, and we might have synced a lot of
> - * newly appeared dirty pages, but have not synced all
> - * of the old dirty pages.
> - */
> - if (mpd->wbc->sync_mode == WB_SYNC_NONE &&
> - mpd->next_page - mpd->first_page >=
> - mpd->wbc->nr_to_write)
> - goto out;
> + left--;
> }
> pagevec_release(&pvec);
> cond_resched();
>
>
> > Attached patch should do what's needed. Can you try whether
> > it fixes the problem for you (it seems to work OK in my testing).
>
> In fact, I had wrote and tested your attached patch before my last post,
> and it may trigger BUG() in mpage_release_unused_pages(), that is because
> we touch mpd->next_page without locking current page, so it is better to
> not increase mpd->next_page if the current page won't be processed.
>
>
> Thanks,
> --
> Ming Lei
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR