| Message ID | 20240328163424.2781320-4-dhowells@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | netfs, afs, 9p, cifs: Rework netfs to use ->writepages() to copy to cache | expand |
On Thu, 2024-03-28 at 16:33 +0000, David Howells wrote: > Update i_blocks when i_size is updated when we finish making a write to the > pagecache to reflect the amount of space we think will be consumed. > Umm ok, but why? I get that the i_size and i_blocks would be out of sync until we get back new attrs from the server, but is that a problem? I'm mainly curious as to what's paying attention to the i_blocks during this window. > Signed-off-by: David Howells <dhowells@redhat.com> > cc: Steve French <sfrench@samba.org> > cc: Shyam Prasad N <nspmangalore@gmail.com> > cc: Rohith Surabattula <rohiths.msft@gmail.com> > cc: Jeff Layton <jlayton@kernel.org> > cc: linux-cifs@vger.kernel.org > cc: netfs@lists.linux.dev > cc: linux-fsdevel@vger.kernel.org > cc: linux-mm@kvack.org > --- > fs/netfs/buffered_write.c | 45 +++++++++++++++++++++++++++++---------- > 1 file changed, 34 insertions(+), 11 deletions(-) > > diff --git a/fs/netfs/buffered_write.c b/fs/netfs/buffered_write.c > index 9a0d32e4b422..c194655a6dcf 100644 > --- a/fs/netfs/buffered_write.c > +++ b/fs/netfs/buffered_write.c > @@ -130,6 +130,37 @@ static struct folio *netfs_grab_folio_for_write(struct address_space *mapping, > mapping_gfp_mask(mapping)); > } > > +/* > + * Update i_size and estimate the update to i_blocks to reflect the additional > + * data written into the pagecache until we can find out from the server what > + * the values actually are. > + */ > +static void netfs_update_i_size(struct netfs_inode *ctx, struct inode *inode, > + loff_t i_size, loff_t pos, size_t copied) > +{ > + blkcnt_t add; > + size_t gap; > + > + if (ctx->ops->update_i_size) { > + ctx->ops->update_i_size(inode, pos); > + return; > + } > + > + i_size_write(inode, pos); > +#if IS_ENABLED(CONFIG_FSCACHE) > + fscache_update_cookie(ctx->cache, NULL, &pos); > +#endif > + > + gap = SECTOR_SIZE - (i_size & (SECTOR_SIZE - 1)); > + if (copied > gap) { > + add = DIV_ROUND_UP(copied - gap, SECTOR_SIZE); > + > + inode->i_blocks = min_t(blkcnt_t, > + DIV_ROUND_UP(pos, SECTOR_SIZE), > + inode->i_blocks + add); > + } > +} > + > /** > * netfs_perform_write - Copy data into the pagecache. > * @iocb: The operation parameters > @@ -352,18 +383,10 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter, > trace_netfs_folio(folio, trace); > > /* Update the inode size if we moved the EOF marker */ > - i_size = i_size_read(inode); > pos += copied; > - if (pos > i_size) { > - if (ctx->ops->update_i_size) { > - ctx->ops->update_i_size(inode, pos); > - } else { > - i_size_write(inode, pos); > -#if IS_ENABLED(CONFIG_FSCACHE) > - fscache_update_cookie(ctx->cache, NULL, &pos); > -#endif > - } > - } > + i_size = i_size_read(inode); > + if (pos > i_size) > + netfs_update_i_size(ctx, inode, i_size, pos, copied); > written += copied; > > if (likely(!wreq)) { > Reviewed-by: Jeff Layton <jlayton@kernel.org>
Jeff Layton <jlayton@kernel.org> wrote: > > Update i_blocks when i_size is updated when we finish making a write to the > > pagecache to reflect the amount of space we think will be consumed. > > > > Umm ok, but why? I get that the i_size and i_blocks would be out of sync > until we get back new attrs from the server, but is that a problem? I'm > mainly curious as to what's paying attention to the i_blocks during this > window. This is taking over from a cifs patch that does the same thing - but in code that is removed by my cifs-netfs branch, so I should probably let Steve speak to that, though I think the problem with cifs is that these fields aren't properly updated until the closure occurs and the server is consulted. commit dbfdff402d89854126658376cbcb08363194d3cd Author: Steve French <stfrench@microsoft.com> Date: Thu Feb 22 00:26:52 2024 -0600 smb3: update allocation size more accurately on write completion Changes to allocation size are approximated for extending writes of cached files until the server returns the actual value (on SMB3 close or query info for example), but it was setting the estimated value for number of blocks to larger than the file size even if the file is likely sparse which breaks various xfstests (e.g. generic/129, 130, 221, 228). When i_size and i_blocks are updated in write completion do not increase allocation size more than what was written (rounded up to 512 bytes). David
diff --git a/fs/netfs/buffered_write.c b/fs/netfs/buffered_write.c index 9a0d32e4b422..c194655a6dcf 100644 --- a/fs/netfs/buffered_write.c +++ b/fs/netfs/buffered_write.c @@ -130,6 +130,37 @@ static struct folio *netfs_grab_folio_for_write(struct address_space *mapping, mapping_gfp_mask(mapping)); } +/* + * Update i_size and estimate the update to i_blocks to reflect the additional + * data written into the pagecache until we can find out from the server what + * the values actually are. + */ +static void netfs_update_i_size(struct netfs_inode *ctx, struct inode *inode, + loff_t i_size, loff_t pos, size_t copied) +{ + blkcnt_t add; + size_t gap; + + if (ctx->ops->update_i_size) { + ctx->ops->update_i_size(inode, pos); + return; + } + + i_size_write(inode, pos); +#if IS_ENABLED(CONFIG_FSCACHE) + fscache_update_cookie(ctx->cache, NULL, &pos); +#endif + + gap = SECTOR_SIZE - (i_size & (SECTOR_SIZE - 1)); + if (copied > gap) { + add = DIV_ROUND_UP(copied - gap, SECTOR_SIZE); + + inode->i_blocks = min_t(blkcnt_t, + DIV_ROUND_UP(pos, SECTOR_SIZE), + inode->i_blocks + add); + } +} + /** * netfs_perform_write - Copy data into the pagecache. * @iocb: The operation parameters @@ -352,18 +383,10 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter, trace_netfs_folio(folio, trace); /* Update the inode size if we moved the EOF marker */ - i_size = i_size_read(inode); pos += copied; - if (pos > i_size) { - if (ctx->ops->update_i_size) { - ctx->ops->update_i_size(inode, pos); - } else { - i_size_write(inode, pos); -#if IS_ENABLED(CONFIG_FSCACHE) - fscache_update_cookie(ctx->cache, NULL, &pos); -#endif - } - } + i_size = i_size_read(inode); + if (pos > i_size) + netfs_update_i_size(ctx, inode, i_size, pos, copied); written += copied; if (likely(!wreq)) {
Update i_blocks when i_size is updated when we finish making a write to the pagecache to reflect the amount of space we think will be consumed. Signed-off-by: David Howells <dhowells@redhat.com> cc: Steve French <sfrench@samba.org> cc: Shyam Prasad N <nspmangalore@gmail.com> cc: Rohith Surabattula <rohiths.msft@gmail.com> cc: Jeff Layton <jlayton@kernel.org> cc: linux-cifs@vger.kernel.org cc: netfs@lists.linux.dev cc: linux-fsdevel@vger.kernel.org cc: linux-mm@kvack.org --- fs/netfs/buffered_write.c | 45 +++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 11 deletions(-)