Re: [NFS] [PATCH 2/2] nfs4: on a O_EXCL OPEN make sure the SETATTR sets the fields holding the verifier

[Trond Myklebust <trond.myklebust@fys.uio.no>]

On Tue, 2007-06-05 at 13:56 -0400, Jeff Layton wrote:
> The Linux NFS4 client simply skips over the bitmask in an O_EXCL open
> call and so it doesn't bother to reset any fields that may be holding
> the verifier. This patch has us save the first two words of the bitmask
> (which is all the current client has #defines for). The client then
> later checks this bitmask and turns on the appropriate flags in the
> sattr->ia_verify field for the following SETATTR call.
> 
> This patch only currently checks to see if the server used the atime
> and mtime slots for the verifier (which is what the Linux server uses
> for this). I'm not sure of what other fields the server could
> reasonably use, but adding checks for others should be trivial.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 8e46e3e..34ddd66 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -955,6 +955,22 @@ static struct nfs4_state *nfs4_open_delegated(struct inode *inode, int flags, st
>  }
>  
>  /*
> + * on an EXCLUSIVE create, the server should send back a bitmask with FATTR4-*
> + * fields corresponding to attributes that were used to store the verifier.
> + * Make sure we clobber those fields in the later setattr call
> + */
> +static inline void nfs4_exclusive_attrset (struct nfs4_opendata *opendata, struct iattr *sattr)
> +{
> +	if ((opendata->o_res.attrset[1] & FATTR4_WORD1_TIME_ACCESS) &&
> +	    !(sattr->ia_valid & ATTR_ATIME_SET))
> +		sattr->ia_valid |= ATTR_ATIME;
> +
> +	if ((opendata->o_res.attrset[1] & FATTR4_WORD1_TIME_MODIFY) &&
> +	    !(sattr->ia_valid & ATTR_MTIME_SET))
> +		sattr->ia_valid |= ATTR_MTIME;
> +}
> +
> +/*
>   * Returns a referenced nfs4_state
>   */
>  static int _nfs4_do_open(struct inode *dir, struct dentry *dentry, int flags, struct iattr *sattr, struct rpc_cred *cred, struct nfs4_state **res)
> @@ -985,6 +1001,9 @@ static int _nfs4_do_open(struct inode *dir, struct dentry *dentry, int flags, st
>  	if (status != 0)
>  		goto err_opendata_free;
>  
> +	if (opendata->o_arg.open_flags & O_EXCL)
> +		nfs4_exclusive_attrset(opendata, sattr);
> +
>  	status = -ENOMEM;
>  	state = nfs4_opendata_to_nfs4_state(opendata);
>  	if (state == NULL)
> @@ -1787,6 +1806,7 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
>  		status = nfs4_do_setattr(state->inode, &fattr, sattr, state);
>  		if (status == 0)
>  			nfs_setattr_update_inode(state->inode, sattr);
> +		nfs_refresh_inode(state->inode, &fattr);
                  ^^^^ should be nfs_post_op_update_inode() to ensure
that the attribute cache gets invalidated if the server fails to return
the post-op attributes.

>  	}
>  	if (status == 0 && nd != NULL && (nd->flags & LOOKUP_OPEN))
>  		status = nfs4_intent_set_file(nd, dentry, state);
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 4eb8a59..b281c83 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -3313,7 +3313,7 @@ static int decode_delegation(struct xdr_stream *xdr, struct nfs_openres *res)
>  static int decode_open(struct xdr_stream *xdr, struct nfs_openres *res)
>  {
>          __be32 *p;
> -        uint32_t bmlen;
> +        uint32_t savewords, bmlen, i;
>          int status;
>  
>          status = decode_op_hdr(xdr, OP_OPEN);
> @@ -3331,7 +3331,15 @@ static int decode_open(struct xdr_stream *xdr, struct nfs_openres *res)
>                  goto xdr_error;
>  
>          READ_BUF(bmlen << 2);
> -        p += bmlen;
> +
> +	savewords = bmlen > NFS_OPENRES_ATTRSET_WORDS ?
> +			NFS_OPENRES_ATTRSET_WORDS : bmlen;
                      ^^^^ min_t(bmlen, NFS_OPENRES_ATTRSET_WORDS, uint32_t);
> +
> +	for (i = 0; i < savewords; ++i) 
> +		READ32(res->attrset[i]);
> +
> +	p += (bmlen - savewords);
> +
>  	return decode_delegation(xdr, res);
>  xdr_error:
>  	dprintk("%s: Bitmap too large! Length = %u\n", __FUNCTION__, bmlen);
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index f8c55e5..f050a27 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -133,6 +133,7 @@ struct nfs_openargs {
>  	__u32			claim;
>  };
>  
> +#define NFS_OPENRES_ATTRSET_WORDS	2

   Could you rename this to NFS4_BITMAP_SIZE and put it into
include/linux/nfs4.h, please.

>  struct nfs_openres {
>  	nfs4_stateid            stateid;
>  	struct nfs_fh           fh;
> @@ -145,6 +146,7 @@ struct nfs_openres {
>  	nfs4_stateid		delegation;
>  	__u32			do_recall;
>  	__u64			maxsize;
> +	__u32			attrset[NFS_OPENRES_ATTRSET_WORDS];
>  };
>  
>  /*
> 
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> _______________________________________________
> NFS maillist  -  NFS@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nfs

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/