Received-SPF: pass (google.com: domain of linux-nfs+bounces-2579-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 1 Apr 2024 14:53:51 +0100
From: Simon Horman <horms@kernel.org>
To: David Howells <dhowells@redhat.com>
Cc: Christian Brauner <christian@brauner.io>,
	Jeff Layton <jlayton@kernel.org>,
	Gao Xiang <hsiangkao@linux.alibaba.com>,
	Dominique Martinet <asmadeus@codewreck.org>,
	Matthew Wilcox <willy@infradead.org>,
	Steve French <smfrench@gmail.com>,
	Marc Dionne <marc.dionne@auristor.com>,
	Paulo Alcantara <pc@manguebit.com>,
	Shyam Prasad N <sprasad@microsoft.com>, Tom Talpey <tom@talpey.com>,
	Eric Van Hensbergen <ericvh@kernel.org>,
	Ilya Dryomov <idryomov@gmail.com>, netfs@lists.linux.dev,
	linux-cachefs@redhat.com, linux-afs@lists.infradead.org,
	linux-cifs@vger.kernel.org, linux-nfs@vger.kernel.org,
	ceph-devel@vger.kernel.org, v9fs@lists.linux.dev,
	linux-erofs@lists.ozlabs.org, linux-fsdevel@vger.kernel.org,
	linux-mm@kvack.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 26/26] netfs, afs: Use writeback retry to deal with
 alternate keys
Message-ID: <20240401135351.GD26556@kernel.org>
References: <20240328163424.2781320-1-dhowells@redhat.com>
 <20240328163424.2781320-27-dhowells@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240328163424.2781320-27-dhowells@redhat.com>

On Thu, Mar 28, 2024 at 04:34:18PM +0000, David Howells wrote:

...

> +void afs_issue_write(struct netfs_io_subrequest *subreq)
>  {
> +	struct netfs_io_request *wreq = subreq->rreq;
>  	struct afs_operation *op;
> -	struct afs_wb_key *wbk = NULL;
> -	loff_t size = iov_iter_count(iter);
> +	struct afs_vnode *vnode = AFS_FS_I(wreq->inode);
> +	unsigned long long pos = subreq->start + subreq->transferred;
> +	size_t len = subreq->len - subreq->transferred;
>  	int ret = -ENOKEY;
>  
> -	_enter("%s{%llx:%llu.%u},%llx,%llx",
> +	_enter("R=%x[%x],%s{%llx:%llu.%u},%llx,%zx",
> +	       wreq->debug_id, subreq->debug_index,
>  	       vnode->volume->name,
>  	       vnode->fid.vid,
>  	       vnode->fid.vnode,
>  	       vnode->fid.unique,
> -	       size, pos);
> +	       pos, len);
>  
> -	ret = afs_get_writeback_key(vnode, &wbk);
> -	if (ret) {
> -		_leave(" = %d [no keys]", ret);
> -		return ret;
> -	}
> +#if 0 // Error injection
> +	if (subreq->debug_index == 3)
> +		return netfs_write_subrequest_terminated(subreq, -ENOANO, false);
>  
> -	op = afs_alloc_operation(wbk->key, vnode->volume);
> -	if (IS_ERR(op)) {
> -		afs_put_wb_key(wbk);
> -		return -ENOMEM;
> +	if (!test_bit(NETFS_SREQ_RETRYING, &subreq->flags)) {
> +		set_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
> +		return netfs_write_subrequest_terminated(subreq, -EAGAIN, false);
>  	}
> +#endif
> +
> +	op = afs_alloc_operation(wreq->netfs_priv, vnode->volume);
> +	if (IS_ERR(op))
> +		return netfs_write_subrequest_terminated(subreq, -EAGAIN, false);
>  
>  	afs_op_set_vnode(op, 0, vnode);
> -	op->file[0].dv_delta = 1;
> +	op->file[0].dv_delta	= 1;
>  	op->file[0].modification = true;
> -	op->store.pos = pos;
> -	op->store.size = size;
> -	op->flags |= AFS_OPERATION_UNINTR;
> -	op->ops = &afs_store_data_operation;
> +	op->store.pos		= pos;
> +	op->store.size		= len,

nit: this is probably more intuitively written using len;

> +	op->flags		|= AFS_OPERATION_UNINTR;
> +	op->ops			= &afs_store_data_operation;
>  
> -try_next_key:
>  	afs_begin_vnode_operation(op);
>  
> -	op->store.write_iter = iter;
> -	op->store.i_size = max(pos + size, vnode->netfs.remote_i_size);
> -	op->mtime = inode_get_mtime(&vnode->netfs.inode);
> +	op->store.write_iter	= &subreq->io_iter;
> +	op->store.i_size	= umax(pos + len, vnode->netfs.remote_i_size);
> +	op->mtime		= inode_get_mtime(&vnode->netfs.inode);
>  
>  	afs_wait_for_operation(op);
> -
> -	switch (afs_op_error(op)) {
> +	ret = afs_put_operation(op);
> +	switch (ret) {
>  	case -EACCES:
>  	case -EPERM:
>  	case -ENOKEY:
>  	case -EKEYEXPIRED:
>  	case -EKEYREJECTED:
>  	case -EKEYREVOKED:
> -		_debug("next");
> -
> -		ret = afs_get_writeback_key(vnode, &wbk);
> -		if (ret == 0) {
> -			key_put(op->key);
> -			op->key = key_get(wbk->key);
> -			goto try_next_key;
> -		}
> +		/* If there are more keys we can try, use the retry algorithm
> +		 * to rotate the keys.
> +		 */
> +		if (wreq->netfs_priv2)
> +			set_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
>  		break;
>  	}
>  
> -	afs_put_wb_key(wbk);
> -	_leave(" = %d", afs_op_error(op));
> -	return afs_put_operation(op);
> +	netfs_write_subrequest_terminated(subreq, ret < 0 ? ret : subreq->len, false);
>  }
>  
>  /*

...