Re: unionfs: several more problems

On Mon, 26 Nov 2007, Erez Zadok wrote:

I just posted a series of patches to unionfs (already in unionfs.git on
korg), which fix every problem LTP found, as well as other problems
mentioned in your email. With this series of patches, the same set of tests
which pass on ext3 also pass with unionfs mounted over ext3.

Good, I'll give them a try, but not tonight - my own bugs to work on!


I'd like to call your attention to one small unionfs_writepage fix. One of
the ltp fs tests (rwtest04) triggered a BUG_ON(PageWriteback(page)) in
fs/buffer.c:1706, when we call the lower ->writepage. This was due to
multiple writers to the same page. Looking at other kernel code, it seems
that the right fix is to use wait_on_page_writeback(lower_page) to serialize
concurrent writebacks to the same page, no?

Ah, yes. I believe that's the right thing to do (and you do have the
lower_page locked there, which is necessary for that serialization).

The small patch below fixed the problem. Let me know what you think.

I've one issue with it: please move that wait_on_page_writeback before
the clear_page_dirty_for_io instead of after it, then resubmit your 14/16.

As it stands, you're still mixing the end of the previous writeback with
the beginning of the new one (e.g. clear_page_dirty_for_io is clearing
the PageReclaim flag when it shouldn't be interfering at all); and the
page might get dirtied during that wait for the previous writeback to
complete, in which case you'd end up doing an extra unnecessary write
(because page later found "dirty" even though written back long after
last modification).

Whether this is a big deal in any way, I don't know; but if you look
at other instances, you'll find the wait_on_page_writeback is done
before the clear_page_dirty_for_io, so you'll surely be on safer
ground to follow the same ordering.

(And so far as I can see, everyone who calls write_one_page does so
with the wait flag 1: that's the clearest example. Hmm, I wonder if
its initial wait_on_page_writeback is right to be conditional on wait
at all. Well, until someone calls it with wait 0, it's hard to tell.)

Hugh


Thanks,
Erez.


diff --git a/fs/unionfs/mmap.c b/fs/unionfs/mmap.c
index eef43aa..d0fd4d0 100644
--- a/fs/unionfs/mmap.c
+++ b/fs/unionfs/mmap.c
@@ -73,6 +73,7 @@ static int unionfs_writepage(struct page *page, struct writeback_control *wbc)

BUG_ON(!lower_mapping->a_ops->writepage);
clear_page_dirty_for_io(lower_page); /* emulate VFS behavior */
+ wait_on_page_writeback(lower_page); /* prevent multiple writers */
err = lower_mapping->a_ops->writepage(lower_page, wbc);
if (err < 0)
goto out_release;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: unionfs: several more problems
    ... I just posted a series of patches to unionfs (already in unionfs.git on ... korg), which fix every problem LTP found, as well as other problems ... I'd like to call your attention to one small unionfs_writepage fix. ...
    (Linux-Kernel)
  • [ANNOUNCE] Stacked GIT 0.13
    ... operations are performed using GIT commands and the patches are stored ... Safety checks for the 'rebase' command ... already modified by the current patch ... Fix bash completion to not garble the screen with an error message. ...
    (Linux-Kernel)
  • 2.6.21-mm2
    ... A handful of IDE patches were dropped due to compilation failures. ... See the `hot-fixes' directory for any important updates to this patchset. ... If you hit a bug in -mm and it is not obvious which patch caused it, ... sunrpc fix ...
    (Linux-Kernel)
  • Re: Performance regression in scsi sequential throughput (iozone) due to "e084b - page-alloc
    ... But both patches reverted at the same time on git v2.6.32 bring us back to our old values! ... But I don't think un-applying it is really a fix as no one really understands how e084b cause this regression. ... trying high-order allocations depending on availability. ... A patch is on its way upstream to ...
    (Linux-Kernel)
  • Re: [PATCH 2.6.27.y 1/3] ext4: Use our own write_cache_pages()
    ... regression fixes in later commits, ... The first two of these three patches really only exist as support for the last one, which fixes the deadlock that I was really concerned about. ... While the other patches may fix some real bugs on .27, ...
    (Linux-Kernel)