diff mbox series

[v2,19/19] afs: Maintain netfs_i_context::remote_i_size

Message ID 164678220204.1200972.17408022517463940584.stgit@warthog.procyon.org.uk (mailing list archive)
State New, archived
Headers show
Series netfs: Prep for write helpers | expand

Commit Message

David Howells March 8, 2022, 11:30 p.m. UTC
Make afs use netfslib's tracking for the server's idea of what the current
inode size is independently of inode->i_size.  We really want to use this
value when calculating the new vnode size when initiating a StoreData RPC
op rather than the size stat() presents to the user (ie. inode->i_size) as
the latter is affected by as-yet uncommitted writes.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: linux-cachefs@redhat.com
cc: linux-afs@lists.infradead.org

Link: https://lore.kernel.org/r/164623014626.3564931.8375344024648265358.stgit@warthog.procyon.org.uk/ # v1
---

 fs/afs/inode.c |    1 +
 fs/afs/write.c |    7 +++----
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Jeff Layton March 9, 2022, 9:12 p.m. UTC | #1
On Tue, 2022-03-08 at 23:30 +0000, David Howells wrote:
> Make afs use netfslib's tracking for the server's idea of what the current
> inode size is independently of inode->i_size.  We really want to use this
> value when calculating the new vnode size when initiating a StoreData RPC
> op rather than the size stat() presents to the user (ie. inode->i_size) as
> the latter is affected by as-yet uncommitted writes.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: linux-cachefs@redhat.com
> cc: linux-afs@lists.infradead.org
> 
> Link: https://lore.kernel.org/r/164623014626.3564931.8375344024648265358.stgit@warthog.procyon.org.uk/ # v1
> ---
> 
>  fs/afs/inode.c |    1 +
>  fs/afs/write.c |    7 +++----
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/afs/inode.c b/fs/afs/inode.c
> index 5b5e40197655..2fe402483ad5 100644
> --- a/fs/afs/inode.c
> +++ b/fs/afs/inode.c
> @@ -246,6 +246,7 @@ static void afs_apply_status(struct afs_operation *op,
>  		 * idea of what the size should be that's not the same as
>  		 * what's on the server.
>  		 */
> +		vnode->netfs_ctx.remote_i_size = status->size;
>  		if (change_size) {
>  			afs_set_i_size(vnode, status->size);
>  			inode->i_ctime = t;
> diff --git a/fs/afs/write.c b/fs/afs/write.c
> index e4b47f67a408..85c9056ba9fb 100644
> --- a/fs/afs/write.c
> +++ b/fs/afs/write.c
> @@ -353,9 +353,10 @@ static const struct afs_operation_ops afs_store_data_operation = {
>  static int afs_store_data(struct afs_vnode *vnode, struct iov_iter *iter, loff_t pos,
>  			  bool laundering)
>  {
> +	struct netfs_i_context *ictx = &vnode->netfs_ctx;
>  	struct afs_operation *op;
>  	struct afs_wb_key *wbk = NULL;
> -	loff_t size = iov_iter_count(iter), i_size;
> +	loff_t size = iov_iter_count(iter);
>  	int ret = -ENOKEY;
>  
>  	_enter("%s{%llx:%llu.%u},%llx,%llx",
> @@ -377,15 +378,13 @@ static int afs_store_data(struct afs_vnode *vnode, struct iov_iter *iter, loff_t
>  		return -ENOMEM;
>  	}
>  
> -	i_size = i_size_read(&vnode->vfs_inode);
> -
>  	afs_op_set_vnode(op, 0, vnode);
>  	op->file[0].dv_delta = 1;
>  	op->file[0].modification = true;
>  	op->store.write_iter = iter;
>  	op->store.pos = pos;
>  	op->store.size = size;
> -	op->store.i_size = max(pos + size, i_size);
> +	op->store.i_size = max(pos + size, ictx->remote_i_size);

Ahh ok, so if i_size is larger than is represented by this write, you'll
have a zeroed out region until writeback catches up. Makes sense.

>  	op->store.laundering = laundering;
>  	op->mtime = vnode->vfs_inode.i_mtime;
>  	op->flags |= AFS_OPERATION_UNINTR;
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>
David Howells March 9, 2022, 10:35 p.m. UTC | #2
Jeff Layton <jlayton@kernel.org> wrote:

> > -	op->store.i_size = max(pos + size, i_size);
> > +	op->store.i_size = max(pos + size, ictx->remote_i_size);
> 
> Ahh ok, so if i_size is larger than is represented by this write, you'll
> have a zeroed out region until writeback catches up. Makes sense.

That's the way it was working.  With this change, we track the server's idea
of the file size separately from our local inode->i_size (which is updated by
the modifications into the pagecache) and only expand the server's setting to
the end of the data we're storing, not to our local i_size.  I'm trying to
avoid zeroed-out regions appearing in the file.

Forcible expansion by truncate is a different matter.

David
diff mbox series

Patch

diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 5b5e40197655..2fe402483ad5 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -246,6 +246,7 @@  static void afs_apply_status(struct afs_operation *op,
 		 * idea of what the size should be that's not the same as
 		 * what's on the server.
 		 */
+		vnode->netfs_ctx.remote_i_size = status->size;
 		if (change_size) {
 			afs_set_i_size(vnode, status->size);
 			inode->i_ctime = t;
diff --git a/fs/afs/write.c b/fs/afs/write.c
index e4b47f67a408..85c9056ba9fb 100644
--- a/fs/afs/write.c
+++ b/fs/afs/write.c
@@ -353,9 +353,10 @@  static const struct afs_operation_ops afs_store_data_operation = {
 static int afs_store_data(struct afs_vnode *vnode, struct iov_iter *iter, loff_t pos,
 			  bool laundering)
 {
+	struct netfs_i_context *ictx = &vnode->netfs_ctx;
 	struct afs_operation *op;
 	struct afs_wb_key *wbk = NULL;
-	loff_t size = iov_iter_count(iter), i_size;
+	loff_t size = iov_iter_count(iter);
 	int ret = -ENOKEY;
 
 	_enter("%s{%llx:%llu.%u},%llx,%llx",
@@ -377,15 +378,13 @@  static int afs_store_data(struct afs_vnode *vnode, struct iov_iter *iter, loff_t
 		return -ENOMEM;
 	}
 
-	i_size = i_size_read(&vnode->vfs_inode);
-
 	afs_op_set_vnode(op, 0, vnode);
 	op->file[0].dv_delta = 1;
 	op->file[0].modification = true;
 	op->store.write_iter = iter;
 	op->store.pos = pos;
 	op->store.size = size;
-	op->store.i_size = max(pos + size, i_size);
+	op->store.i_size = max(pos + size, ictx->remote_i_size);
 	op->store.laundering = laundering;
 	op->mtime = vnode->vfs_inode.i_mtime;
 	op->flags |= AFS_OPERATION_UNINTR;