| Message ID | 162391825688.1173366.3437507255136307904.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | netfs, afs: Fix netfs_write_begin and THP handling | expand |
On Thu, Jun 17, 2021 at 09:24:16AM +0100, David Howells wrote: > Fix afs_write_end() to correctly handle a short copy into the intended > write region of the page. Two things are necessary: > > (1) If the page is not up to date, then we should just return 0 > (ie. indicating a zero-length copy). The loop in > generic_perform_write() will go around again, possibly breaking up the > iterator into discrete chunks. > > This is analogous to commit b9de313cf05fe08fa59efaf19756ec5283af672a > for ceph. > > (2) The page should not have been set uptodate if it wasn't completely set > up by netfs_write_begin() (this will be fixed in the next patch), so > we need to set uptodate here in such a case. > > Also remove the assertion that was checking that the page was set uptodate > since it's now set uptodate if it wasn't already a few lines above. The > assertion was from when uptodate was set elsewhere. Thanks for adding that explanation. > +++ b/fs/afs/write.c > @@ -119,6 +119,16 @@ int afs_write_end(struct file *file, struct address_space *mapping, > _enter("{%llx:%llu},{%lx}", > vnode->fid.vid, vnode->fid.vnode, page->index); > > + len = min_t(size_t, len, thp_size(page) - from); This line isn't necessary yet, right? > + if (!PageUptodate(page)) { > + if (copied < len) { > + copied = 0; > + goto out; > + } > + > + SetPageUptodate(page); > + } > + > if (copied == 0) > goto out; > > @@ -133,8 +143,6 @@ int afs_write_end(struct file *file, struct address_space *mapping, > write_sequnlock(&vnode->cb_lock); > } > > - ASSERT(PageUptodate(page)); > - > if (PagePrivate(page)) { > priv = page_private(page); > f = afs_page_dirty_from(page, priv); The rest of this looks good.
diff --git a/fs/afs/write.c b/fs/afs/write.c index 56e2cff2cb87..8a4053ae03dd 100644 --- a/fs/afs/write.c +++ b/fs/afs/write.c @@ -119,6 +119,16 @@ int afs_write_end(struct file *file, struct address_space *mapping, _enter("{%llx:%llu},{%lx}", vnode->fid.vid, vnode->fid.vnode, page->index); + len = min_t(size_t, len, thp_size(page) - from); + if (!PageUptodate(page)) { + if (copied < len) { + copied = 0; + goto out; + } + + SetPageUptodate(page); + } + if (copied == 0) goto out; @@ -133,8 +143,6 @@ int afs_write_end(struct file *file, struct address_space *mapping, write_sequnlock(&vnode->cb_lock); } - ASSERT(PageUptodate(page)); - if (PagePrivate(page)) { priv = page_private(page); f = afs_page_dirty_from(page, priv);
Fix afs_write_end() to correctly handle a short copy into the intended write region of the page. Two things are necessary: (1) If the page is not up to date, then we should just return 0 (ie. indicating a zero-length copy). The loop in generic_perform_write() will go around again, possibly breaking up the iterator into discrete chunks. This is analogous to commit b9de313cf05fe08fa59efaf19756ec5283af672a for ceph. (2) The page should not have been set uptodate if it wasn't completely set up by netfs_write_begin() (this will be fixed in the next patch), so we need to set uptodate here in such a case. Also remove the assertion that was checking that the page was set uptodate since it's now set uptodate if it wasn't already a few lines above. The assertion was from when uptodate was set elsewhere. Fixes: 3003bbd0697b ("afs: Use the netfs_write_begin() helper") Reported-by: Jeff Layton <jlayton@kernel.org> Signed-off-by: David Howells <dhowells@redhat.com> Acked-by: Jeff Layton <jlayton@kernel.org> cc: Al Viro <viro@zeniv.linux.org.uk> cc: Matthew Wilcox <willy@infradead.org> cc: linux-afs@lists.infradead.org Link: https://lore.kernel.org/r/162367682522.460125.5652091227576721609.stgit@warthog.procyon.org.uk/ # v1 --- fs/afs/write.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)