| Message ID | 163975498535.2021751.13839139728966985077.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] ceph: Uninline the data on a file opened for writing | expand |
On Fri, 2021-12-17 at 15:29 +0000, David Howells wrote: > If a ceph file is made up of inline data, uninline that in the ceph_open() > rather than in ceph_page_mkwrite(), ceph_write_iter(), ceph_fallocate() or > ceph_write_begin(). > > This makes it easier to convert to using the netfs library for VM write > hooks. > > Changes > ======= > ver #2: > - Removed the uninline-handling code from ceph_write_begin() also. > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: ceph-devel@vger.kernel.org > --- > > fs/ceph/addr.c | 97 +++++++++++++------------------------------------------ > fs/ceph/file.c | 28 +++++++++------- > fs/ceph/super.h | 2 + > 3 files changed, 40 insertions(+), 87 deletions(-) > > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c > index e836f8f1d4f8..6e1b15cc87cf 100644 > --- a/fs/ceph/addr.c > +++ b/fs/ceph/addr.c > @@ -1278,45 +1278,11 @@ static int ceph_write_begin(struct file *file, struct address_space *mapping, > struct page **pagep, void **fsdata) > { > struct inode *inode = file_inode(file); > - struct ceph_inode_info *ci = ceph_inode(inode); > struct folio *folio = NULL; > - pgoff_t index = pos >> PAGE_SHIFT; > int r; > > - /* > - * Uninlining should have already been done and everything updated, EXCEPT > - * for inline_version sent to the MDS. > - */ > - if (ci->i_inline_version != CEPH_INLINE_NONE) { > - unsigned int fgp_flags = FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE; > - if (aop_flags & AOP_FLAG_NOFS) > - fgp_flags |= FGP_NOFS; > - folio = __filemap_get_folio(mapping, index, fgp_flags, > - mapping_gfp_mask(mapping)); > - if (!folio) > - return -ENOMEM; > - > - /* > - * The inline_version on a new inode is set to 1. If that's the > - * case, then the folio is brand new and isn't yet Uptodate. > - */ > - r = 0; > - if (index == 0 && ci->i_inline_version != 1) { > - if (!folio_test_uptodate(folio)) { > - WARN_ONCE(1, "ceph: write_begin called on still-inlined inode (inline_version %llu)!\n", > - ci->i_inline_version); > - r = -EINVAL; > - } > - goto out; > - } > - zero_user_segment(&folio->page, 0, folio_size(folio)); > - folio_mark_uptodate(folio); > - goto out; > - } > - > r = netfs_write_begin(file, inode->i_mapping, pos, len, 0, &folio, NULL, > &ceph_netfs_read_ops, NULL); > -out: > if (r == 0) > folio_wait_fscache(folio); > if (r < 0) { > @@ -1512,19 +1478,6 @@ static vm_fault_t ceph_page_mkwrite(struct vm_fault *vmf) > sb_start_pagefault(inode->i_sb); > ceph_block_sigs(&oldset); > > - if (ci->i_inline_version != CEPH_INLINE_NONE) { > - struct page *locked_page = NULL; > - if (off == 0) { > - lock_page(page); > - locked_page = page; > - } > - err = ceph_uninline_data(vma->vm_file, locked_page); > - if (locked_page) > - unlock_page(locked_page); > - if (err < 0) > - goto out_free; > - } > - > if (off + thp_size(page) <= size) > len = thp_size(page); > else > @@ -1649,13 +1602,14 @@ void ceph_fill_inline_data(struct inode *inode, struct page *locked_page, > } > } > > -int ceph_uninline_data(struct file *filp, struct page *locked_page) > +int ceph_uninline_data(struct file *file) > { > - struct inode *inode = file_inode(filp); > + struct inode *inode = file_inode(file); > struct ceph_inode_info *ci = ceph_inode(inode); > struct ceph_fs_client *fsc = ceph_inode_to_client(inode); > struct ceph_osd_request *req; > - struct page *page = NULL; > + struct folio *folio = NULL; > + struct page *pages[1]; > u64 len, inline_version; > int err = 0; > bool from_pagecache = false; > @@ -1671,34 +1625,30 @@ int ceph_uninline_data(struct file *filp, struct page *locked_page) > inline_version == CEPH_INLINE_NONE) > goto out; > > - if (locked_page) { > - page = locked_page; > - WARN_ON(!PageUptodate(page)); > - } else if (ceph_caps_issued(ci) & > - (CEPH_CAP_FILE_CACHE|CEPH_CAP_FILE_LAZYIO)) { > - page = find_get_page(inode->i_mapping, 0); > - if (page) { > - if (PageUptodate(page)) { > + if (ceph_caps_issued(ci) & (CEPH_CAP_FILE_CACHE|CEPH_CAP_FILE_LAZYIO)) { > + folio = filemap_get_folio(inode->i_mapping, 0); > + if (folio) { > + if (folio_test_uptodate(folio)) { > from_pagecache = true; > - lock_page(page); > + folio_lock(folio); > } else { > - put_page(page); > - page = NULL; > + folio_put(folio); > + folio = NULL; > } > } > } > > - if (page) { > + if (folio) { > len = i_size_read(inode); > - if (len > PAGE_SIZE) > - len = PAGE_SIZE; > + if (len > folio_size(folio)) > + len = folio_size(folio); > } else { > - page = __page_cache_alloc(GFP_NOFS); > - if (!page) { > + folio = filemap_alloc_folio(GFP_NOFS, 0); > + if (!folio) { > err = -ENOMEM; > goto out; > } > - err = __ceph_do_getattr(inode, page, > + err = __ceph_do_getattr(inode, folio_page(folio, 0), > CEPH_STAT_CAP_INLINE_DATA, true); > if (err < 0) { > /* no inline data */ > @@ -1736,7 +1686,8 @@ int ceph_uninline_data(struct file *filp, struct page *locked_page) > goto out; > } > > - osd_req_op_extent_osd_data_pages(req, 1, &page, len, 0, false, false); > + pages[0] = folio_page(folio, 0); > + osd_req_op_extent_osd_data_pages(req, 1, pages, len, 0, false, false); > > { > __le64 xattr_buf = cpu_to_le64(inline_version); > @@ -1773,12 +1724,10 @@ int ceph_uninline_data(struct file *filp, struct page *locked_page) > if (err == -ECANCELED) > err = 0; > out: > - if (page && page != locked_page) { > - if (from_pagecache) { > - unlock_page(page); > - put_page(page); > - } else > - __free_pages(page, 0); > + if (folio) { > + if (from_pagecache) > + folio_unlock(folio); > + folio_put(folio); > } > > dout("uninline_data %p %llx.%llx inline_version %llu = %d\n", > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index bf1017682d09..d16ba8720783 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -205,6 +205,7 @@ static int ceph_init_file_info(struct inode *inode, struct file *file, > { > struct ceph_inode_info *ci = ceph_inode(inode); > struct ceph_file_info *fi; > + int ret; > > dout("%s %p %p 0%o (%s)\n", __func__, inode, file, > inode->i_mode, isdir ? "dir" : "regular"); > @@ -235,7 +236,22 @@ static int ceph_init_file_info(struct inode *inode, struct file *file, > INIT_LIST_HEAD(&fi->rw_contexts); > fi->filp_gen = READ_ONCE(ceph_inode_to_client(inode)->filp_gen); > > + if ((file->f_mode & FMODE_WRITE) && > + ci->i_inline_version != CEPH_INLINE_NONE) { > + ret = ceph_uninline_data(file); > + if (ret < 0) > + goto error; > + } > + > return 0; > + > +error: > + ceph_fscache_unuse_cookie(inode, file->f_mode & FMODE_WRITE); > + ceph_put_fmode(ci, fi->fmode, 1); > + kmem_cache_free(ceph_file_cachep, fi); > + /* wake up anyone waiting for caps on this inode */ > + wake_up_all(&ci->i_cap_wq); > + return ret; > } > > /* > @@ -1751,12 +1767,6 @@ static ssize_t ceph_write_iter(struct kiocb *iocb, struct iov_iter *from) > if (err) > goto out; > > - if (ci->i_inline_version != CEPH_INLINE_NONE) { > - err = ceph_uninline_data(file, NULL); > - if (err < 0) > - goto out; > - } > - > dout("aio_write %p %llx.%llx %llu~%zd getting caps. i_size %llu\n", > inode, ceph_vinop(inode), pos, count, i_size_read(inode)); > if (fi->fmode & CEPH_FILE_MODE_LAZY) > @@ -2082,12 +2092,6 @@ static long ceph_fallocate(struct file *file, int mode, > goto unlock; > } > > - if (ci->i_inline_version != CEPH_INLINE_NONE) { > - ret = ceph_uninline_data(file, NULL); > - if (ret < 0) > - goto unlock; > - } > - > size = i_size_read(inode); > > /* Are we punching a hole beyond EOF? */ > diff --git a/fs/ceph/super.h b/fs/ceph/super.h > index d0142cc5c41b..f1cec05e4eb8 100644 > --- a/fs/ceph/super.h > +++ b/fs/ceph/super.h > @@ -1207,7 +1207,7 @@ extern void __ceph_touch_fmode(struct ceph_inode_info *ci, > /* addr.c */ > extern const struct address_space_operations ceph_aops; > extern int ceph_mmap(struct file *file, struct vm_area_struct *vma); > -extern int ceph_uninline_data(struct file *filp, struct page *locked_page); > +extern int ceph_uninline_data(struct file *file); > extern int ceph_pool_perm_check(struct inode *inode, int need); > extern void ceph_pool_perm_destroy(struct ceph_mds_client* mdsc); > int ceph_purge_inode_cap(struct inode *inode, struct ceph_cap *cap, bool *invalidate); > > It's a substantial reduction in code and gets the uninlining out of the write codepaths. I like it. Looks like this relies on the changes in your fscache-rewrite branch, so we may need to wait until after the upcoming merge window to take this in, or just plan to fold your fscache-rewrite branch into the ceph testing branch for now. I'll at least do some local testing in the interim, but it's not trivial to set up inlining these days, so we may not be able to test what this affects easily.
On Fri, Dec 17, 2021 at 03:29:45PM +0000, David Howells wrote: > If a ceph file is made up of inline data, uninline that in the ceph_open() > rather than in ceph_page_mkwrite(), ceph_write_iter(), ceph_fallocate() or > ceph_write_begin(). I don't think this is the right approach. Just opening a file with O_RDWR shouldn't take it out of inline mode; an actual write (or fault) should be required to uninline it. > This makes it easier to convert to using the netfs library for VM write > hooks. I don't understand. You're talking about the fault path? Surely the filesystem gets called with the vm_fault parameter only, then calls into the netfs code, passing vmf and the operations struct? And ceph could uninline there.
On Fri, 2021-12-17 at 16:20 +0000, Matthew Wilcox wrote: > On Fri, Dec 17, 2021 at 03:29:45PM +0000, David Howells wrote: > > If a ceph file is made up of inline data, uninline that in the ceph_open() > > rather than in ceph_page_mkwrite(), ceph_write_iter(), ceph_fallocate() or > > ceph_write_begin(). > > I don't think this is the right approach. Just opening a file with > O_RDWR shouldn't take it out of inline mode; an actual write (or fault) > should be required to uninline it. > This feature is being deprecated in ceph altogether, so more aggressively uninlining of files is fine. The kernel cephfs client never supported writes to it anyway so this feature was really only used by a few brave souls. We're hoping to have it formally removed by the time the Ceph Quincy release ships (~April-May timeframe). Unfortunately, we need to keep support for it around for a bit longer since some still-supported ceph releases don't have this deprecated. > > This makes it easier to convert to using the netfs library for VM write > > hooks. > > I don't understand. You're talking about the fault path? Surely > the filesystem gets called with the vm_fault parameter only, then > calls into the netfs code, passing vmf and the operations struct? > And ceph could uninline there. > Taking the uninlining out of the write codepaths is a _good_ thing, IMO. If we were planning to keep this feature around, then I might disagree with doing this, but it fits with the current plans for inline data just fine for now.
On Fri, Dec 17, 2021 at 12:18:04PM -0500, Jeff Layton wrote: > This feature is being deprecated in ceph altogether, so more > aggressively uninlining of files is fine. The kernel cephfs client never > supported writes to it anyway so this feature was really only used by a > few brave souls. > > We're hoping to have it formally removed by the time the Ceph Quincy > release ships (~April-May timeframe). Unfortunately, we need to keep > support for it around for a bit longer since some still-supported ceph > releases don't have this deprecated. OK, I shan't bother pointing out the awful bugs and races in the current uninlining code then ...
diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c index e836f8f1d4f8..6e1b15cc87cf 100644 --- a/fs/ceph/addr.c +++ b/fs/ceph/addr.c @@ -1278,45 +1278,11 @@ static int ceph_write_begin(struct file *file, struct address_space *mapping, struct page **pagep, void **fsdata) { struct inode *inode = file_inode(file); - struct ceph_inode_info *ci = ceph_inode(inode); struct folio *folio = NULL; - pgoff_t index = pos >> PAGE_SHIFT; int r; - /* - * Uninlining should have already been done and everything updated, EXCEPT - * for inline_version sent to the MDS. - */ - if (ci->i_inline_version != CEPH_INLINE_NONE) { - unsigned int fgp_flags = FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE; - if (aop_flags & AOP_FLAG_NOFS) - fgp_flags |= FGP_NOFS; - folio = __filemap_get_folio(mapping, index, fgp_flags, - mapping_gfp_mask(mapping)); - if (!folio) - return -ENOMEM; - - /* - * The inline_version on a new inode is set to 1. If that's the - * case, then the folio is brand new and isn't yet Uptodate. - */ - r = 0; - if (index == 0 && ci->i_inline_version != 1) { - if (!folio_test_uptodate(folio)) { - WARN_ONCE(1, "ceph: write_begin called on still-inlined inode (inline_version %llu)!\n", - ci->i_inline_version); - r = -EINVAL; - } - goto out; - } - zero_user_segment(&folio->page, 0, folio_size(folio)); - folio_mark_uptodate(folio); - goto out; - } - r = netfs_write_begin(file, inode->i_mapping, pos, len, 0, &folio, NULL, &ceph_netfs_read_ops, NULL); -out: if (r == 0) folio_wait_fscache(folio); if (r < 0) { @@ -1512,19 +1478,6 @@ static vm_fault_t ceph_page_mkwrite(struct vm_fault *vmf) sb_start_pagefault(inode->i_sb); ceph_block_sigs(&oldset); - if (ci->i_inline_version != CEPH_INLINE_NONE) { - struct page *locked_page = NULL; - if (off == 0) { - lock_page(page); - locked_page = page; - } - err = ceph_uninline_data(vma->vm_file, locked_page); - if (locked_page) - unlock_page(locked_page); - if (err < 0) - goto out_free; - } - if (off + thp_size(page) <= size) len = thp_size(page); else @@ -1649,13 +1602,14 @@ void ceph_fill_inline_data(struct inode *inode, struct page *locked_page, } } -int ceph_uninline_data(struct file *filp, struct page *locked_page) +int ceph_uninline_data(struct file *file) { - struct inode *inode = file_inode(filp); + struct inode *inode = file_inode(file); struct ceph_inode_info *ci = ceph_inode(inode); struct ceph_fs_client *fsc = ceph_inode_to_client(inode); struct ceph_osd_request *req; - struct page *page = NULL; + struct folio *folio = NULL; + struct page *pages[1]; u64 len, inline_version; int err = 0; bool from_pagecache = false; @@ -1671,34 +1625,30 @@ int ceph_uninline_data(struct file *filp, struct page *locked_page) inline_version == CEPH_INLINE_NONE) goto out; - if (locked_page) { - page = locked_page; - WARN_ON(!PageUptodate(page)); - } else if (ceph_caps_issued(ci) & - (CEPH_CAP_FILE_CACHE|CEPH_CAP_FILE_LAZYIO)) { - page = find_get_page(inode->i_mapping, 0); - if (page) { - if (PageUptodate(page)) { + if (ceph_caps_issued(ci) & (CEPH_CAP_FILE_CACHE|CEPH_CAP_FILE_LAZYIO)) { + folio = filemap_get_folio(inode->i_mapping, 0); + if (folio) { + if (folio_test_uptodate(folio)) { from_pagecache = true; - lock_page(page); + folio_lock(folio); } else { - put_page(page); - page = NULL; + folio_put(folio); + folio = NULL; } } } - if (page) { + if (folio) { len = i_size_read(inode); - if (len > PAGE_SIZE) - len = PAGE_SIZE; + if (len > folio_size(folio)) + len = folio_size(folio); } else { - page = __page_cache_alloc(GFP_NOFS); - if (!page) { + folio = filemap_alloc_folio(GFP_NOFS, 0); + if (!folio) { err = -ENOMEM; goto out; } - err = __ceph_do_getattr(inode, page, + err = __ceph_do_getattr(inode, folio_page(folio, 0), CEPH_STAT_CAP_INLINE_DATA, true); if (err < 0) { /* no inline data */ @@ -1736,7 +1686,8 @@ int ceph_uninline_data(struct file *filp, struct page *locked_page) goto out; } - osd_req_op_extent_osd_data_pages(req, 1, &page, len, 0, false, false); + pages[0] = folio_page(folio, 0); + osd_req_op_extent_osd_data_pages(req, 1, pages, len, 0, false, false); { __le64 xattr_buf = cpu_to_le64(inline_version); @@ -1773,12 +1724,10 @@ int ceph_uninline_data(struct file *filp, struct page *locked_page) if (err == -ECANCELED) err = 0; out: - if (page && page != locked_page) { - if (from_pagecache) { - unlock_page(page); - put_page(page); - } else - __free_pages(page, 0); + if (folio) { + if (from_pagecache) + folio_unlock(folio); + folio_put(folio); } dout("uninline_data %p %llx.%llx inline_version %llu = %d\n", diff --git a/fs/ceph/file.c b/fs/ceph/file.c index bf1017682d09..d16ba8720783 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -205,6 +205,7 @@ static int ceph_init_file_info(struct inode *inode, struct file *file, { struct ceph_inode_info *ci = ceph_inode(inode); struct ceph_file_info *fi; + int ret; dout("%s %p %p 0%o (%s)\n", __func__, inode, file, inode->i_mode, isdir ? "dir" : "regular"); @@ -235,7 +236,22 @@ static int ceph_init_file_info(struct inode *inode, struct file *file, INIT_LIST_HEAD(&fi->rw_contexts); fi->filp_gen = READ_ONCE(ceph_inode_to_client(inode)->filp_gen); + if ((file->f_mode & FMODE_WRITE) && + ci->i_inline_version != CEPH_INLINE_NONE) { + ret = ceph_uninline_data(file); + if (ret < 0) + goto error; + } + return 0; + +error: + ceph_fscache_unuse_cookie(inode, file->f_mode & FMODE_WRITE); + ceph_put_fmode(ci, fi->fmode, 1); + kmem_cache_free(ceph_file_cachep, fi); + /* wake up anyone waiting for caps on this inode */ + wake_up_all(&ci->i_cap_wq); + return ret; } /* @@ -1751,12 +1767,6 @@ static ssize_t ceph_write_iter(struct kiocb *iocb, struct iov_iter *from) if (err) goto out; - if (ci->i_inline_version != CEPH_INLINE_NONE) { - err = ceph_uninline_data(file, NULL); - if (err < 0) - goto out; - } - dout("aio_write %p %llx.%llx %llu~%zd getting caps. i_size %llu\n", inode, ceph_vinop(inode), pos, count, i_size_read(inode)); if (fi->fmode & CEPH_FILE_MODE_LAZY) @@ -2082,12 +2092,6 @@ static long ceph_fallocate(struct file *file, int mode, goto unlock; } - if (ci->i_inline_version != CEPH_INLINE_NONE) { - ret = ceph_uninline_data(file, NULL); - if (ret < 0) - goto unlock; - } - size = i_size_read(inode); /* Are we punching a hole beyond EOF? */ diff --git a/fs/ceph/super.h b/fs/ceph/super.h index d0142cc5c41b..f1cec05e4eb8 100644 --- a/fs/ceph/super.h +++ b/fs/ceph/super.h @@ -1207,7 +1207,7 @@ extern void __ceph_touch_fmode(struct ceph_inode_info *ci, /* addr.c */ extern const struct address_space_operations ceph_aops; extern int ceph_mmap(struct file *file, struct vm_area_struct *vma); -extern int ceph_uninline_data(struct file *filp, struct page *locked_page); +extern int ceph_uninline_data(struct file *file); extern int ceph_pool_perm_check(struct inode *inode, int need); extern void ceph_pool_perm_destroy(struct ceph_mds_client* mdsc); int ceph_purge_inode_cap(struct inode *inode, struct ceph_cap *cap, bool *invalidate);
If a ceph file is made up of inline data, uninline that in the ceph_open() rather than in ceph_page_mkwrite(), ceph_write_iter(), ceph_fallocate() or ceph_write_begin(). This makes it easier to convert to using the netfs library for VM write hooks. Changes ======= ver #2: - Removed the uninline-handling code from ceph_write_begin() also. Signed-off-by: David Howells <dhowells@redhat.com> cc: ceph-devel@vger.kernel.org --- fs/ceph/addr.c | 97 +++++++++++++------------------------------------------ fs/ceph/file.c | 28 +++++++++------- fs/ceph/super.h | 2 + 3 files changed, 40 insertions(+), 87 deletions(-)