On Thu, May 20, 2010 at 10:50 PM, Tao Guo <glorioustao@xxxxxxxxx> wrote:
> On Fri, May 21, 2010 at 12:04 AM, <andros@xxxxxxxxxx> wrote:
>> From: Andy Adamson <andros@xxxxxxxxxx>
>>
>> Taking a reference on the nfs_open_context to signal that a layoutcommit is
>> needed is problematic because the last reference to the context triggers a
>> close (nfs_release). But, if the layout holds a reference on the
>> nfs_open_context, then close will not be triggered.
>>
>> Since we only use the rpc credential from the layoutcommit_ctx, replace the
>> layoutcommit_ctx with the rpc_cred.
>>
>> Hold a reference on the rpc_cred until the layoutcommit rpc returns. Note that
>> the rpc layer (rpcauth_bind) also references the rpc_cred.
>>
>> If the layoutdriver fails to setup the layoutcommit, clear the layoutcommit
>> properties and put the credential.
>>
>> Signed-off-by: Andy Adamson <andros@xxxxxxxxxx>
>> ---
>> fs/nfs/inode.c | 4 ++--
>> fs/nfs/nfs4state.c | 2 +-
>> fs/nfs/pnfs.c | 42 +++++++++++++++++-------------------------
>> fs/nfs/write.c | 2 +-
>> include/linux/nfs4_pnfs.h | 6 ++++++
>> include/linux/nfs_fs.h | 3 +--
>> include/linux/pnfs_xdr.h | 1 -
>> 7 files changed, 28 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>> index 7fd33d9..2b8e8e6 100644
>> --- a/fs/nfs/inode.c
>> +++ b/fs/nfs/inode.c
>> @@ -1110,7 +1110,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>> /*
>> * file needs layout commit, server attributes may be stale
>> */
>> - if (nfsi->layoutcommit_ctx && nfsi->change_attr >= fattr->change_attr) {
>> + if (do_layoutcommit(nfsi) && nfsi->change_attr >= fattr->change_attr) {
>> dprintk("NFS: %s: layoutcommit is needed for file %s/%ld\n",
>> __func__, inode->i_sb->s_id, inode->i_ino);
>> return 0;
>> @@ -1331,7 +1331,7 @@ static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
>> nfsi->pnfs_layout_state = 0;
>> memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
>> nfsi->layout.roc_iomode = 0;
>> - nfsi->layoutcommit_ctx = NULL;
>> + nfsi->lo_cred = NULL;
>> nfsi->pnfs_write_begin_pos = 0;
>> nfsi->pnfs_write_end_pos = 0;
>> #endif /* CONFIG_NFS_V4_1 */
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index d145de1..bf03fde 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -589,7 +589,7 @@ static void __nfs4_close(struct path *path, struct nfs4_state *state, fmode_t fm
>> #ifdef CONFIG_NFS_V4_1
>> struct nfs_inode *nfsi = NFS_I(state->inode);
>>
>> - if (nfsi->layoutcommit_ctx)
>> + if (do_layoutcommit(nfsi))
>> pnfs_layoutcommit_inode(state->inode, wait);
>> if (has_layout(nfsi) && nfsi->layout.roc_iomode) {
>> struct nfs4_pnfs_layout_segment range;
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index 20285bc..cb8ff06 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -145,21 +145,19 @@ find_pnfs(u32 id, struct pnfs_module **module) {
>> return 0;
>> }
>>
>> -/* Set context to indicate we require a layoutcommit
>> +/* Set lo_cred to indicate we require a layoutcommit
>> * If we don't even have a layout, we don't need to commit it.
>> */
>> void
>> pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_context *ctx)
>> {
>> - dprintk("%s: has_layout=%d layoutcommit_ctx=%p ctx=%p\n", __func__,
>> - has_layout(nfsi), nfsi->layoutcommit_ctx, ctx);
>> + dprintk("%s: has_layout=%d ctx=%p\n", __func__, has_layout(nfsi), ctx);
>> spin_lock(&nfsi->lo_lock);
>> - if (has_layout(nfsi) && !nfsi->layoutcommit_ctx) {
>> - nfsi->layoutcommit_ctx = get_nfs_open_context(ctx);
>> + if (has_layout(nfsi) && !do_layoutcommit(nfsi)) {
>> + nfsi->lo_cred = get_rpccred(ctx->state->owner->so_cred);
>> nfsi->change_attr++;
>> spin_unlock(&nfsi->lo_lock);
>> - dprintk("%s: Set layoutcommit_ctx=%p\n", __func__,
>> - nfsi->layoutcommit_ctx);
>> + dprintk("%s: Set layoutcommit\n", __func__);
>> return;
>> }
>> spin_unlock(&nfsi->lo_lock);
>> @@ -755,7 +753,7 @@ _pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
>> !pnfs_return_layout_barrier(nfsi, &arg));
>> }
>>
>> - if (nfsi->layoutcommit_ctx) {
>> + if (do_layoutcommit(nfsi)) {
>> status = pnfs_layoutcommit_inode(ino, wait);
>> if (status) {
>> dprintk("%s: layoutcommit failed, status=%d. "
>> @@ -1910,16 +1908,9 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
>>
>> dprintk("%s: (status %d)\n", __func__, data->status);
>>
>> - /* TODO: For now, set an error in the open context (just like
>> - * if a commit failed) We may want to do more, much more, like
>> - * replay all writes through the NFSv4
>> - * server, or something.
>> - */
>> - if (data->status < 0) {
>> + if (data->status < 0)
>> printk(KERN_ERR "%s, Layoutcommit Failed! = %d\n",
>> __func__, data->status);
>> - data->ctx->error = data->status;
>> - }
>>
>> /* TODO: Maybe we should avoid this by allowing the layout driver
>> * to directly xdr its layout on the wire.
>> @@ -1929,9 +1920,6 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
>> &nfsi->layout,
>> &data->args,
>> data->status);
>> -
>> - /* release the open_context acquired in pnfs_writeback_done */
>> - put_nfs_open_context(data->ctx);
>> }
>>
>> /*
>> @@ -1995,30 +1983,34 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
>> return -ENOMEM;
>>
>> spin_lock(&nfsi->lo_lock);
>> - if (!nfsi->layoutcommit_ctx)
>> + if (!do_layoutcommit(nfsi))
>> goto out_unlock;
>>
>> data->args.inode = inode;
>> - data->cred = nfsi->layoutcommit_ctx->cred;
>> - data->ctx = nfsi->layoutcommit_ctx;
>> + data->cred = nfsi->lo_cred;
>>
>> /* Set up layout commit args*/
>> status = pnfs_layoutcommit_setup(data, sync);
>> - if (status)
>> - goto out_unlock;
>>
>> /* Clear layoutcommit properties in the inode so
>> * new lc info can be generated
>> */
>> nfsi->pnfs_write_begin_pos = 0;
>> nfsi->pnfs_write_end_pos = 0;
>> - nfsi->layoutcommit_ctx = NULL;
>> + nfsi->lo_cred = NULL;
>> +
>> + if (status) {
>> + /* The layout driver failed to setup the layoutcommit */
>> + put_rpccred(data->cred);
>> + goto out_unlock;
>> + }
>>
>> /* release lock on pnfs layoutcommit attrs */
>> spin_unlock(&nfsi->lo_lock);
>>
>> data->is_sync = sync;
>> status = pnfs4_proc_layoutcommit(data);
>> + put_rpccred(data->cred);
> Is this OK to put_rpccred here if sync == 0? why not move it into
> pnfs_layoutcommit_done just as the code did?
As I said in the patch comments, the RPC layer takes a reference on the cred,
-->Andy
> --tao
>
>> out:
>> dprintk("%s end (err:%d)\n", __func__, status);
>> return status;
>> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
>> index a4c95a0..2c1918e 100644
>> --- a/fs/nfs/write.c
>> +++ b/fs/nfs/write.c
>> @@ -1490,7 +1490,7 @@ static int nfs_commit_inode(struct inode *inode, int how)
>> nfs_wait_bit_killable,
>> TASK_KILLABLE);
>> #ifdef CONFIG_NFS_V4_1
>> - if (may_wait && NFS_I(inode)->layoutcommit_ctx) {
>> + if (may_wait && do_layoutcommit(NFS_I(inode))) {
>> error = pnfs_layoutcommit_inode(inode, 1);
>> if (error < 0)
>> return error;
>> diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
>> index 4d47b48..c9ef43e 100644
>> --- a/include/linux/nfs4_pnfs.h
>> +++ b/include/linux/nfs4_pnfs.h
>> @@ -80,6 +80,12 @@ has_layout(struct nfs_inode *nfsi)
>> return nfsi->layout.ld_data != NULL;
>> }
>>
>> +static inline bool
>> +do_layoutcommit(struct nfs_inode *nfsi)
>> +{
>> + return nfsi->lo_cred != NULL;
>> +}
>> +
>> #endif /* CONFIG_NFS_V4_1 */
>>
>> struct pnfs_layout_segment {
>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>> index 98a8dc0..478b00c 100644
>> --- a/include/linux/nfs_fs.h
>> +++ b/include/linux/nfs_fs.h
>> @@ -203,11 +203,10 @@ struct nfs_inode {
>> #define NFS_INO_RW_LAYOUT_FAILED 1 /* get rw layout failed stop trying */
>> #define NFS_INO_LAYOUT_ALLOC 2 /* bit lock for layout allocation */
>> time_t pnfs_layout_suspend;
>> + struct rpc_cred *lo_cred; /* layoutcommit credential */
>> wait_queue_head_t lo_waitq;
>> spinlock_t lo_lock;
>> struct pnfs_layout_type layout;
>> - /* use rpc_creds in this open_context to send LAYOUTCOMMIT to MDS */
>> - struct nfs_open_context *layoutcommit_ctx;
>> /* DH: These vars keep track of the maximum write range
>> * so the values can be used for layoutcommit.
>> */
>> diff --git a/include/linux/pnfs_xdr.h b/include/linux/pnfs_xdr.h
>> index a0bf341..154b04e 100644
>> --- a/include/linux/pnfs_xdr.h
>> +++ b/include/linux/pnfs_xdr.h
>> @@ -86,7 +86,6 @@ struct pnfs_layoutcommit_data {
>> bool is_sync;
>> struct rpc_cred *cred;
>> struct nfs_fattr fattr;
>> - struct nfs_open_context *ctx;
>> struct pnfs_layoutcommit_arg args;
>> struct pnfs_layoutcommit_res res;
>> int status;
>> --
>> 1.6.6
>>
>> --
>> 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
>>
>
>
>
> --
> tao.
> --
> 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
>
--
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
