[pnfs] [PATCH 06/10] nfs41: New backchannel_rqst.c file.

[Benny Halevy]

On Jan. 03, 2008, 2:01 +0200, Ricardo Labiaga <ricardo.labiaga at netapp.com> wrote:
<snip>
> +
> +#ifdef RPC_DEBUG
> +# undef  RPC_DEBUG_DATA

Why is that?
Seems like this line was just copied from net/sunrpc/xprtsock.c
where it has a meaning...

> +# define RPCDBG_FACILITY	RPCDBG_TRANS
> +#endif
> +
> +#if defined(CONFIG_NFS_V4_1)
> +
> +int xprt_setup_backchannel(struct rpc_xprt *xprt, unsigned int min_reqs)
> +{
> +	struct page *page_priv = NULL, *page_snd = NULL;
> +	struct xdr_buf *xbufp = NULL;
> +
> +	BUG_ON(min_reqs > 1);	/* We only prealloate buffers for one slot */
> +	dprintk("RPC:        setup backchannel transport\n");
> +
> +	/* Pre-allocate one backchannel rpc_rqst */
> +	xprt->bc_rpc_rqst = kmalloc(sizeof(struct rpc_rqst), GFP_KERNEL);
> +	if (xprt->bc_rpc_rqst == NULL) {
> +		printk(KERN_ERR "Failed to create backchannel rpc_rqst\n");
> +		goto out_free;
> +	}
> +	xprt->bc_rpc_rqst->rq_xprt = xprt;
> +
> +	/* Preallocate one XDR private buffer */
> +	page_priv = alloc_page(GFP_KERNEL);
> +	if (page_priv == NULL) {
> +		printk(KERN_ERR "Failed to create backchannel priv xbuf\n");
> +		goto out_free;
> +	}
> +	xbufp = &xprt->bc_rpc_rqst->rq_private_buf;
> +	xbufp->head[0].iov_base = page_address(page_priv);
> +	xbufp->head[0].iov_len = PAGE_SIZE;

Shouldn't xbufp->tail[0].iov_base set to NULL?
(Even if setting tail[0].iov_len to zero is sufficient in the current code base
I think it's safer for forward compatibility to initialize iov_base as well)

> +	xbufp->tail[0].iov_len = 0;
> +	xbufp->page_len = 0;
> +	xbufp->len = PAGE_SIZE;
> +	xbufp->buflen = PAGE_SIZE;
> +
> +	/* Preallocate one XDR send buffer */
> +	page_snd = alloc_page(GFP_KERNEL);
> +	if (page_snd == NULL) {
> +		printk(KERN_ERR "Failed to create backchannel snd xbuf\n");
> +		goto out_free;
> +	}
> +
> +	xbufp = &xprt->bc_rpc_rqst->rq_snd_buf;
> +	xbufp->head[0].iov_base = page_address(page_snd);
> +	xbufp->head[0].iov_len = 0;

ditto

> +	xbufp->tail[0].iov_len = 0;
> +	xbufp->page_len = 0;
> +	xbufp->len = 0;
> +	xbufp->buflen = PAGE_SIZE;
> +
> +	dprintk("RPC:        setup backchannel transport done\n");
> +	return 0;
> +
> +out_free:
> +	kfree(page_snd);
> +	kfree(page_priv);
> +	kfree(xprt->bc_rpc_rqst);
> +
> +	dprintk("RPC:        setup backchannel transport failed\n");
> +	return -1;
> +}
> +EXPORT_SYMBOL(xprt_setup_backchannel);
> +
> +void xprt_destroy_backchannel(struct rpc_xprt *xprt)

Hmm, I see no-one calling xprt_destroy_backchannel...
Is the nfs41 callback_down path planned to be implemented
in a later stage?

> +{
> +	struct rpc_rqst *req = xprt->bc_rpc_rqst;
> +	struct xdr_buf *xbufp;
> +
> +	dprintk("RPC:        destroy backchannel transport\n");
> +	/*
> +	 * Any of these can be NULL if the user hit ^C
> +	 */
> +	if (req == NULL)
> +		return;		/* Nothing to do */

should we set xprt->bc_rpc_rqst = NULL here?
Ideally, atomic swapping could be nice if you expect parallel
destroyers, which I presume we don't here, but if this
function can ever be called again at a later time we don't
want it to free things twice...

> +	xbufp = &req->rq_private_buf;
> +	if (xbufp)

How can xbufp be NULL?

> +		kfree(xbufp->head[0].iov_base);
> +	xbufp = &req->rq_snd_buf;
> +	if (xbufp)

ditto

> +		kfree(xbufp->head[0].iov_base);
> +	kfree(xprt->bc_rpc_rqst);
> +}
> +EXPORT_SYMBOL(xprt_destroy_backchannel);
> +
> +/*
> + * A single rpc_rqst structure has been preallocated during the backchannel
> + * setup.  Buffer space for the send and private XDR buffers has been
> + * preallocated as well.  Use xprt_alloc_bc_request to allocate this
> + * space for this request.  Use xprt_free_bc_request to return it.
> + *
> + * Returns the rpc_rqst if it's not in use, otherwise NULL.
> + */
> +struct rpc_rqst *xprt_alloc_bc_request(struct rpc_xprt *xprt)
> +{
> +	struct rpc_rqst *req;
> +
> +	dprintk("RPC:        allocate a backchannel request\n");
> +	req = test_and_set_bit(RPC_BC_PREALLOC_IN_USE, &xprt->bc_flags) ?
> +		NULL : xprt->bc_rpc_rqst;

better use test_and_set_bit_lock here and clear_bit_unlock in 
xprt_free_bc_request.

> +
> +	dprintk("RPC:        backchannel req=%p\n", req);
> +	return req;
> +}
> +
> +/*
> + * Return the preallocated rpc_rqst structure and XDR buffers
> + */
> +void xprt_free_bc_request(struct rpc_rqst *req)
> +{
> +	struct rpc_xprt *xprt = req->rq_xprt;
> +	dprintk("RPC:        free backchannel req=%p\n", req);
> +
> +	smp_mb__before_clear_bit();
> +	clear_bit(RPC_BC_PREALLOC_IN_USE, &xprt->bc_flags);
> +	smp_mb__after_clear_bit();

see comment above.
I believe that clear_bit_unlock is the modern way of doing just that.

> +
> +}
> +EXPORT_SYMBOL(xprt_free_bc_request);
> +
> +#endif /* CONFIG_NFS_V4_1 */