On 2011-05-25 19:40, Boaz Harrosh wrote:
>
> In _pnfs_return_layout:
>
> lrp pointer is checked for NULL after it was already accessed.
>
> The rational here is that in _pnfs_return_layout we want to
> de-ref and release the layout regardless of if we sent the
> return or not (forgetfull). An eventual recall can return -ENOMATCHING
> instead of -EDELAY.
>
> So to keep the reasoning above, copy the stateid twice.
>
> Benny if it is OK to not release the layout on -ENOMEM then the check
> could just be moved above the spin_lock(), and the put_layout_hdr removed.
>
> Also the error returns would leak the lrp so fix it.
>
> Signed-off-by: Boaz Harrosh <bharrosh@xxxxxxxxxxx>
> ---
> fs/nfs/pnfs.c | 15 +++++++++------
> 1 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index a07b007..9b749f2 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -627,22 +627,20 @@ _pnfs_return_layout(struct inode *ino)
> struct pnfs_layout_hdr *lo = NULL;
> struct nfs_inode *nfsi = NFS_I(ino);
> LIST_HEAD(tmp_list);
> - struct nfs4_layoutreturn *lrp;
> + struct nfs4_layoutreturn *lrp = NULL;
> + nfs4_stateid stateid;
> int status = 0;
>
> dprintk("--> %s\n", __func__);
>
> - lrp = kzalloc(sizeof(*lrp), GFP_KERNEL);
> -
> spin_lock(&ino->i_lock);
> lo = nfsi->layout;
> if (!lo || !mark_matching_lsegs_invalid(lo, &tmp_list, NULL)) {
> spin_unlock(&ino->i_lock);
> dprintk("%s: no layout segments to return\n", __func__);
> - kfree(lrp);
> goto out;
> }
> - lrp->args.stateid = nfsi->layout->plh_stateid;
> + stateid = nfsi->layout->plh_stateid;
> /* Reference matched in nfs4_layoutreturn_release */
> get_layout_hdr(lo);
> spin_unlock(&ino->i_lock);
> @@ -650,11 +648,14 @@ _pnfs_return_layout(struct inode *ino)
>
> WARN_ON(test_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->flags));
>
> - if (lrp == NULL) {
I prefer to simply move this test up before the condition calling
mark_matching_lsegs_invalid
> + /* lrp is freed in nfs4_layoutreturn_release */
> + lrp = kzalloc(sizeof(*lrp), GFP_KERNEL);
> + if (unlikely(!lrp)) {
> put_layout_hdr(NFS_I(ino)->layout);
> status = -ENOMEM;
> goto out;
> }
> + lrp->args.stateid = stateid;
> lrp->args.reclaim = 0;
> lrp->args.layout_type = NFS_SERVER(ino)->pnfs_curr_ld->id;
> lrp->args.inode = ino;
> @@ -662,6 +663,8 @@ _pnfs_return_layout(struct inode *ino)
>
> status = nfs4_proc_layoutreturn(lrp);
> out:
> + if (unlikely(status))
> + kfree(lrp);
I wonder where this leak you're seeing is coming from.
rpc_release is supposed to be called even on task allocation error,
see rpc_new_task.
Benny
> dprintk("<-- %s status: %d\n", __func__, status);
> return status;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
