From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Maxim V. Patlasov" Subject: Re: [PATCH 09/14] fuse: Implement writepages and write_begin/write_end callbacks - v2 Date: Wed, 27 Mar 2013 16:39:09 +0400 Message-ID: <5152E86D.4090400@parallels.com> References: <20130125181700.10037.29163.stgit@maximpc.sw.ru> <20130125182426.10037.96089.stgit@maximpc.sw.ru> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: , , , , , , , To: Miklos Szeredi Return-path: Received: from relay.parallels.com ([195.214.232.42]:42611 "EHLO relay.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750811Ab3C0Mig (ORCPT ); Wed, 27 Mar 2013 08:38:36 -0400 In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Hi Miklos, 01/30/2013 03:08 AM, Miklos Szeredi =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > On Fri, Jan 25, 2013 at 7:25 PM, Maxim V. Patlasov > wrote: >> The .writepages one is required to make each writeback request carry= more than >> one page on it. >> >> Changed in v2: >> - fixed fuse_prepare_write() to avoid reads beyond EOF >> - fixed fuse_prepare_write() to zero uninitialized part of page >> >> Original patch by: Pavel Emelyanov >> Signed-off-by: Maxim V. Patlasov >> --- >> fs/fuse/file.c | 282 ++++++++++++++++++++++++++++++++++++++++++++= ++++++++++++ >> 1 files changed, 281 insertions(+), 1 deletions(-) >> >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c >> index 496e74c..3b4dc98 100644 >> --- a/fs/fuse/file.c >> +++ b/fs/fuse/file.c >> @@ -722,7 +722,10 @@ static void fuse_send_readpages(struct fuse_req= *req, struct file *file) >> >> struct fuse_fill_data { >> struct fuse_req *req; >> - struct file *file; >> + union { >> + struct file *file; >> + struct fuse_file *ff; >> + }; >> struct inode *inode; >> unsigned nr_pages; >> }; >> @@ -1530,6 +1533,280 @@ static int fuse_writepage(struct page *page,= struct writeback_control *wbc) >> return err; >> } >> >> +static int fuse_send_writepages(struct fuse_fill_data *data) >> +{ >> + int i, all_ok =3D 1; >> + struct fuse_req *req =3D data->req; >> + struct inode *inode =3D data->inode; >> + struct backing_dev_info *bdi =3D inode->i_mapping->backing_d= ev_info; >> + struct fuse_conn *fc =3D get_fuse_conn(inode); >> + struct fuse_inode *fi =3D get_fuse_inode(inode); >> + loff_t off =3D -1; >> + >> + if (!data->ff) >> + data->ff =3D fuse_write_file(fc, fi); >> + >> + if (!data->ff) { >> + for (i =3D 0; i < req->num_pages; i++) >> + end_page_writeback(req->pages[i]); >> + return -EIO; >> + } >> + >> + req->inode =3D inode; >> + req->misc.write.in.offset =3D page_offset(req->pages[0]); >> + >> + spin_lock(&fc->lock); >> + list_add(&req->writepages_entry, &fi->writepages); >> + spin_unlock(&fc->lock); >> + >> + for (i =3D 0; i < req->num_pages; i++) { >> + struct page *page =3D req->pages[i]; >> + struct page *tmp_page; >> + >> + tmp_page =3D 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_T= EMP); >> + } else >> + all_ok =3D 0; >> + req->pages[i] =3D tmp_page; >> + if (i =3D=3D 0) >> + off =3D page_offset(page); >> + >> + end_page_writeback(page); >> + } >> + >> + if (!all_ok) { >> + for (i =3D 0; i < req->num_pages; i++) { >> + struct page *page =3D req->pages[i]; >> + if (page) { >> + dec_bdi_stat(bdi, BDI_WRITEBACK); >> + dec_zone_page_state(page, NR_WRITEBA= CK_TEMP); >> + __free_page(page); >> + req->pages[i] =3D NULL; >> + } >> + } >> + >> + spin_lock(&fc->lock); >> + list_del(&req->writepages_entry); >> + wake_up(&fi->page_waitq); >> + spin_unlock(&fc->lock); >> + return -ENOMEM; >> + } >> + >> + req->ff =3D fuse_file_get(data->ff); >> + fuse_write_fill(req, data->ff, off, 0); >> + >> + req->misc.write.in.write_flags |=3D FUSE_WRITE_CACHE; >> + req->in.argpages =3D 1; >> + fuse_page_descs_length_init(req, 0, req->num_pages); >> + req->end =3D fuse_writepage_end; >> + >> + spin_lock(&fc->lock); >> + list_add_tail(&req->list, &fi->queued_writes); >> + fuse_flush_writepages(data->inode); >> + spin_unlock(&fc->lock); >> + >> + return 0; >> +} >> + >> +static int fuse_writepages_fill(struct page *page, >> + struct writeback_control *wbc, void *_data) >> +{ >> + struct fuse_fill_data *data =3D _data; >> + struct fuse_req *req =3D data->req; >> + struct inode *inode =3D data->inode; >> + struct fuse_conn *fc =3D get_fuse_conn(inode); >> + >> + if (fuse_page_is_writeback(inode, page->index)) { >> + if (wbc->sync_mode !=3D WB_SYNC_ALL) { >> + redirty_page_for_writepage(wbc, page); >> + unlock_page(page); >> + return 0; >> + } >> + fuse_wait_on_page_writeback(inode, page->index); >> + } >> + >> + if (req->num_pages && >> + (req->num_pages =3D=3D FUSE_MAX_PAGES_PER_REQ || >> + (req->num_pages + 1) * PAGE_CACHE_SIZE > fc->max_write = || >> + req->pages[req->num_pages - 1]->index + 1 !=3D page->in= dex)) { >> + int err; >> + >> + err =3D fuse_send_writepages(data); >> + if (err) { >> + unlock_page(page); >> + return err; >> + } >> + >> + data->req =3D req =3D >> + fuse_request_alloc_nofs(FUSE_MAX_PAGES_PER_R= EQ); >> + if (!req) { >> + unlock_page(page); >> + return -ENOMEM; >> + } >> + } >> + >> + req->pages[req->num_pages] =3D page; >> + req->num_pages++; >> + >> + if (test_set_page_writeback(page)) >> + BUG(); >> + >> + unlock_page(page); >> + >> + return 0; >> +} >> + >> +static int fuse_writepages(struct address_space *mapping, >> + struct writeback_control *wbc) >> +{ >> + struct inode *inode =3D mapping->host; >> + struct fuse_conn *fc =3D get_fuse_conn(inode); >> + struct fuse_fill_data data; >> + int err; >> + >> + if (!fc->writeback_cache) >> + return generic_writepages(mapping, wbc); >> + >> + err =3D -EIO; >> + if (is_bad_inode(inode)) >> + goto out; >> + >> + data.ff =3D NULL; >> + data.inode =3D inode; >> + data.req =3D fuse_request_alloc_nofs(FUSE_MAX_PAGES_PER_REQ)= ; >> + err =3D -ENOMEM; >> + if (!data.req) >> + goto out_put; >> + >> + err =3D write_cache_pages(mapping, wbc, fuse_writepages_fill= , &data); >> + if (data.req) { >> + if (!err && data.req->num_pages) { >> + err =3D fuse_send_writepages(&data); >> + if (err) >> + fuse_put_request(fc, data.req); >> + } else >> + fuse_put_request(fc, data.req); >> + } >> +out_put: >> + if (data.ff) >> + fuse_file_put(data.ff, false); >> +out: >> + return err; >> +} >> + >> +/* >> + * Determine the number of bytes of data the page contains >> + */ >> +static inline unsigned fuse_page_length(struct page *page) >> +{ >> + loff_t i_size =3D i_size_read(page_file_mapping(page)->host)= ; >> + >> + if (i_size > 0) { >> + pgoff_t page_index =3D page_file_index(page); >> + pgoff_t end_index =3D (i_size - 1) >> PAGE_CACHE_SHI= =46T; >> + if (page_index < end_index) >> + return PAGE_CACHE_SIZE; >> + if (page_index =3D=3D end_index) >> + return ((i_size - 1) & ~PAGE_CACHE_MASK) + 1= ; >> + } >> + return 0; >> +} >> + >> +static int fuse_prepare_write(struct fuse_conn *fc, struct file *fi= le, >> + struct page *page, loff_t pos, unsigned len) >> +{ >> + struct fuse_req *req; >> + unsigned num_read; >> + unsigned page_len; >> + int err; >> + >> + if (PageUptodate(page) || (len =3D=3D PAGE_CACHE_SIZE)) >> + return 0; >> + >> + page_len =3D fuse_page_length(page); >> + if (!page_len) { >> + zero_user(page, 0, PAGE_CACHE_SIZE); >> + return 0; >> + } >> + >> + /* >> + * Page writeback can extend beyond the lifetime of the >> + * page-cache page, so make sure we read a properly synced >> + * page. >> + */ >> + fuse_wait_on_page_writeback(page->mapping->host, page->index= ); >> + >> + req =3D fuse_get_req(fc, 1); >> + err =3D PTR_ERR(req); >> + if (IS_ERR(req)) >> + goto out; >> + >> + req->out.page_zeroing =3D 1; >> + req->out.argpages =3D 1; >> + req->num_pages =3D 1; >> + req->pages[0] =3D page; >> + req->page_descs[0].offset =3D 0; >> + req->page_descs[0].length =3D PAGE_SIZE; >> + num_read =3D fuse_send_read(req, file, page_offset(page), pa= ge_len, NULL); >> + err =3D req->out.h.error; >> + fuse_put_request(fc, req); >> +out: >> + if (err) { >> + unlock_page(page); >> + page_cache_release(page); >> + } else if (num_read !=3D PAGE_CACHE_SIZE) { >> + zero_user_segment(page, num_read, PAGE_CACHE_SIZE); >> + } >> + return err; >> +} > Lots of duplication between this and fuse_readpage(). Can some of > that be sanely shared? Yes, I'll factor out the common part. Thanks for finding. > >> + >> +static int fuse_write_begin(struct file *file, struct address_space= *mapping, >> + loff_t pos, unsigned len, unsigned flags, >> + struct page **pagep, void **fsdata) >> +{ >> + pgoff_t index =3D pos >> PAGE_CACHE_SHIFT; >> + struct fuse_conn *fc =3D get_fuse_conn(file->f_dentry->d_ino= de); >> + >> + BUG_ON(!fc->writeback_cache); >> + >> + *pagep =3D grab_cache_page_write_begin(mapping, index, flags= ); >> + if (!*pagep) >> + return -ENOMEM; >> + >> + return fuse_prepare_write(fc, file, *pagep, pos, len); >> +} >> + > One interesting aspect of this writeback-cache case is the handling o= f > "disk full" errors. The write-begin function should make sure that > space is reserved on disk for the write, but this patchset doesn't do > that. It's not a huge issue, but maybe worth thinking about (and a > comment above the fuse_write_begin() function). Good point! I agree that write-begin may be improved in this direction.= =20 I'll add a comment not to forget about it in the future. Thanks, Maxim > >> +static int fuse_commit_write(struct file *file, struct page *page, >> + unsigned from, unsigned to) >> +{ >> + struct inode *inode =3D page->mapping->host; >> + loff_t pos =3D ((loff_t)page->index << PAGE_CACHE_SHIFT) + t= o; >> + >> + if (!PageUptodate(page)) >> + SetPageUptodate(page); >> + >> + fuse_write_update_size(inode, pos); >> + set_page_dirty(page); >> + return 0; >> +} >> + >> +static int fuse_write_end(struct file *file, struct address_space *= mapping, >> + loff_t pos, unsigned len, unsigned copied, >> + struct page *page, void *fsdata) >> +{ >> + unsigned from =3D pos & (PAGE_CACHE_SIZE - 1); >> + >> + fuse_commit_write(file, page, from, from+copied); >> + >> + unlock_page(page); >> + page_cache_release(page); >> + >> + return copied; >> +} >> + >> static int fuse_launder_page(struct page *page) >> { >> int err =3D 0; >> @@ -2436,11 +2713,14 @@ static const struct file_operations fuse_dir= ect_io_file_operations =3D { >> static const struct address_space_operations fuse_file_aops =3D { >> .readpage =3D fuse_readpage, >> .writepage =3D fuse_writepage, >> + .writepages =3D fuse_writepages, >> .launder_page =3D fuse_launder_page, >> .readpages =3D fuse_readpages, >> .set_page_dirty =3D __set_page_dirty_nobuffers, >> .bmap =3D fuse_bmap, >> .direct_IO =3D fuse_direct_IO, >> + .write_begin =3D fuse_write_begin, >> + .write_end =3D fuse_write_end, >> }; >> >> void fuse_init_file_inode(struct inode *inode) >> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html