Hi Miklos,
04/25/2013 02:35 PM, Miklos Szeredi пишет:
On Mon, Apr 1, 2013 at 12:42 PM, Maxim V. Patlasov
<MPatlasov@xxxxxxxxxxxxx> wrote:
The .writepages one is required to make each writeback request carry more than
one page on it.
I'd split this into two parts:
1) implement ->writepages() and enable it unconditionally for mmaped
writeback (why is it not enabled by this patch?)
2) implement ->write_begin() and ->write_end() and conditionally
enable cached writeback
More comments inline.
Thanks a lot for careful review. I agree with most of your comments and
will address them in the next version of patchset. The only point I
disagree is the following:
+
+ spin_lock(&fc->lock);
+ list_add(&req->writepages_entry, &fi->writepages);
+ spin_unlock(&fc->lock);
+
+ for (i = 0; i < req->num_pages; i++) {
+ struct page *page = req->pages[i];
+ struct page *tmp_page;
+
+ tmp_page = alloc_page(GFP_NOFS | __GFP_HIGHMEM);
+ if (tmp_page) {
+ copy_highpage(tmp_page, page);
+ inc_bdi_stat(bdi, BDI_WRITEBACK);
+ inc_zone_page_state(tmp_page, NR_WRITEBACK_TEMP);
Is there a reason why we do the page allocation/copying here instead
of at fill time? I'd guess it would be simpler and more logical
there.
There is a problem to have in mind: we can't call
end_page_writeback(page) before update of fuse writeback state
(fi->writepages). Otherwise a nasty race would be possible when an
activity for that particular page offset intervenes in the middle of
writeback leading to multiple in-flight fuse requests for given page offset.
What you suggested is doable but would require additional space to stash
pointers to original pages because we cannot release them immediately
after copying (due to the problem described above). The size of the
space is not known in advance because we don't know how many pages
write_cache_pages() will process. The size is currently limited by
sizeof(struct page *) * FUSE_MAX_PAGES_PER_REQ, but may become variable
in future (a lot of people complain about 128K limit of fuse request).
Adding dynamically growing array of pages would make the code neither
simpler nor logical.
The approach I implemented utilizes the fact that
fuse_page_is_writeback() and friends require only
req->misc.write.in.offset and req->num_pages to be set correctly. Actual
pointers in req->pages[] doesn't matter. Thus, as soon as the two
parameters are known, I add the request to fi->writepages (blocking
other operations on given page offset) and perform "in place"
allocation/copying avoiding need for extra space to stash page pointers.
Thanks,
Maxim
--
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
