Messages in this thread | | | Date | Fri, 14 Jun 2013 18:03:01 +0400 | From | Maxim Patlasov <> | Subject | Re: [PATCH 09/14] fuse: Implement writepages and write_begin/write_end callbacks - v3 |
| |
Hi Miklos,
04/25/2013 02:35 PM, Miklos Szeredi пишет: > On Mon, Apr 1, 2013 at 12:42 PM, Maxim V. Patlasov > <MPatlasov@parallels.com> 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |