Re: [PATCH 09/14] fuse: Implement writepages and write_begin/write_end callbacks - v3

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux