* [PATCH 1/4] locks: allow support for write delegation
2023-05-11 21:42 [PATCH 0/4] NFSD: add support for NFSv4 write delegation Dai Ngo
@ 2023-05-11 21:43 ` Dai Ngo
2023-05-11 21:43 ` [PATCH 2/4] NFSD: enable " Dai Ngo
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Dai Ngo @ 2023-05-11 21:43 UTC (permalink / raw)
To: chuck.lever, jlayton; +Cc: linux-nfs, linux-fsdevel
Remove the check for F_WRLCK in generic_add_lease to allow file_lock
to be used for write delegation.
NFSD is the first consumer.
Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
fs/locks.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index df8b26a42524..08fb0b4fd4f8 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
if (is_deleg && !inode_trylock(inode))
return -EAGAIN;
- if (is_deleg && arg == F_WRLCK) {
- /* Write delegations are not currently supported: */
- inode_unlock(inode);
- WARN_ON_ONCE(1);
- return -EINVAL;
- }
-
percpu_down_read(&file_rwsem);
spin_lock(&ctx->flc_lock);
time_out_leases(inode, &dispose);
--
2.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/4] NFSD: enable support for write delegation
2023-05-11 21:42 [PATCH 0/4] NFSD: add support for NFSv4 write delegation Dai Ngo
2023-05-11 21:43 ` [PATCH 1/4] locks: allow support for " Dai Ngo
@ 2023-05-11 21:43 ` Dai Ngo
2023-05-11 21:43 ` [PATCH 3/4] NFSD: add supports for CB_GETATTR callback Dai Ngo
2023-05-11 21:43 ` [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation Dai Ngo
3 siblings, 0 replies; 8+ messages in thread
From: Dai Ngo @ 2023-05-11 21:43 UTC (permalink / raw)
To: chuck.lever, jlayton; +Cc: linux-nfs, linux-fsdevel
This patch grants write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
if there is no conflict with other OPENs.
Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
are handled the same as read delegation using notify_change,
try_break_deleg.
Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
fs/nfsd/nfs4state.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6e61fa3acaf1..09a9e16407f9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1144,7 +1144,7 @@ static void block_delegations(struct knfsd_fh *fh)
static struct nfs4_delegation *
alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
- struct nfs4_clnt_odstate *odstate)
+ struct nfs4_clnt_odstate *odstate, u32 dl_type)
{
struct nfs4_delegation *dp;
long n;
@@ -1170,7 +1170,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
INIT_LIST_HEAD(&dp->dl_recall_lru);
dp->dl_clnt_odstate = odstate;
get_clnt_odstate(odstate);
- dp->dl_type = NFS4_OPEN_DELEGATE_READ;
+ dp->dl_type = dl_type;
dp->dl_retries = 1;
dp->dl_recalled = false;
nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
@@ -5451,6 +5451,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
struct nfs4_delegation *dp;
struct nfsd_file *nf;
struct file_lock *fl;
+ u32 deleg;
/*
* The fi_had_conflict and nfs_get_existing_delegation checks
@@ -5460,7 +5461,13 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
if (fp->fi_had_conflict)
return ERR_PTR(-EAGAIN);
- nf = find_readable_file(fp);
+ if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
+ nf = find_writeable_file(fp);
+ deleg = NFS4_OPEN_DELEGATE_WRITE;
+ } else {
+ nf = find_readable_file(fp);
+ deleg = NFS4_OPEN_DELEGATE_READ;
+ }
if (!nf) {
/*
* We probably could attempt another open and get a read
@@ -5491,11 +5498,11 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
return ERR_PTR(status);
status = -ENOMEM;
- dp = alloc_init_deleg(clp, fp, odstate);
+ dp = alloc_init_deleg(clp, fp, odstate, deleg);
if (!dp)
goto out_delegees;
- fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ);
+ fl = nfs4_alloc_init_lease(dp, deleg);
if (!fl)
goto out_clnt_odstate;
@@ -5583,6 +5590,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
struct svc_fh *parent = NULL;
int cb_up;
int status = 0;
+ u32 wdeleg = false;
cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client);
open->op_recall = 0;
@@ -5590,8 +5598,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
case NFS4_OPEN_CLAIM_PREVIOUS:
if (!cb_up)
open->op_recall = 1;
- if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ)
- goto out_no_deleg;
break;
case NFS4_OPEN_CLAIM_NULL:
parent = currentfh;
@@ -5617,7 +5623,9 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
- open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
+ wdeleg = open->op_share_access & NFS4_SHARE_ACCESS_WRITE;
+ open->op_delegate_type = wdeleg ?
+ NFS4_OPEN_DELEGATE_WRITE : NFS4_OPEN_DELEGATE_READ;
nfs4_put_stid(&dp->dl_stid);
return;
out_no_deleg:
--
2.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 3/4] NFSD: add supports for CB_GETATTR callback
2023-05-11 21:42 [PATCH 0/4] NFSD: add support for NFSv4 write delegation Dai Ngo
2023-05-11 21:43 ` [PATCH 1/4] locks: allow support for " Dai Ngo
2023-05-11 21:43 ` [PATCH 2/4] NFSD: enable " Dai Ngo
@ 2023-05-11 21:43 ` Dai Ngo
2023-05-12 15:30 ` Chuck Lever III
2023-05-11 21:43 ` [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation Dai Ngo
3 siblings, 1 reply; 8+ messages in thread
From: Dai Ngo @ 2023-05-11 21:43 UTC (permalink / raw)
To: chuck.lever, jlayton; +Cc: linux-nfs, linux-fsdevel
Includes:
. CB_GETATTR proc for nfs4_cb_procedures[]
. XDR encoding and decoding function for CB_GETATTR request/reply
. add nfs4_cb_fattr to nfs4_delegation for sending CB_GETATTR
and store file attributes from client's reply.
Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
fs/nfsd/nfs4callback.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
fs/nfsd/state.h | 17 +++++++
fs/nfsd/xdr4cb.h | 19 ++++++++
3 files changed, 153 insertions(+)
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 4039ffcf90ba..ca3d72ef5fbc 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -87,6 +87,43 @@ static void encode_bitmap4(struct xdr_stream *xdr, const __u32 *bitmap,
WARN_ON_ONCE(xdr_stream_encode_uint32_array(xdr, bitmap, len) < 0);
}
+static void decode_bitmap4(struct xdr_stream *xdr, __u32 *bitmap,
+ size_t len)
+{
+ WARN_ON_ONCE(xdr_stream_decode_uint32_array(xdr, bitmap, len) < 0);
+}
+
+static int decode_attr_length(struct xdr_stream *xdr, uint32_t *attrlen)
+{
+ __be32 *p;
+
+ p = xdr_inline_decode(xdr, 4);
+ if (unlikely(!p))
+ return -EIO;
+ *attrlen = be32_to_cpup(p);
+ return 0;
+}
+
+static int decode_cb_getattr(struct xdr_stream *xdr, uint32_t *bitmap,
+ struct nfs4_cb_fattr *fattr)
+{
+ __be32 *ptr;
+
+ if (likely(bitmap[0] & FATTR4_WORD0_CHANGE)) {
+ ptr = xdr_inline_decode(xdr, 8);
+ if (unlikely(!ptr))
+ return -EIO;
+ xdr_decode_hyper(ptr, &fattr->ncf_cb_cinfo);
+ }
+ if (likely(bitmap[0] & FATTR4_WORD0_SIZE)) {
+ ptr = xdr_inline_decode(xdr, 8);
+ if (unlikely(!ptr))
+ return -EIO;
+ xdr_decode_hyper(ptr, &fattr->ncf_cb_fsize);
+ }
+ return 0;
+}
+
/*
* nfs_cb_opnum4
*
@@ -358,6 +395,26 @@ encode_cb_recallany4args(struct xdr_stream *xdr,
}
/*
+ * CB_GETATTR4args
+ * struct CB_GETATTR4args {
+ * nfs_fh4 fh;
+ * bitmap4 attr_request;
+ * };
+ *
+ * The size and change attributes are the only one
+ * guaranteed to be serviced by the client.
+ */
+static void
+encode_cb_getattr4args(struct xdr_stream *xdr, struct nfs4_cb_compound_hdr *hdr,
+ struct knfsd_fh *fh, struct nfs4_cb_fattr *fattr)
+{
+ encode_nfs_cb_opnum4(xdr, OP_CB_GETATTR);
+ encode_nfs_fh4(xdr, fh);
+ encode_bitmap4(xdr, fattr->ncf_cb_bmap, ARRAY_SIZE(fattr->ncf_cb_bmap));
+ hdr->nops++;
+}
+
+/*
* CB_SEQUENCE4args
*
* struct CB_SEQUENCE4args {
@@ -493,6 +550,29 @@ static void nfs4_xdr_enc_cb_null(struct rpc_rqst *req, struct xdr_stream *xdr,
}
/*
+ * 20.1. Operation 3: CB_GETATTR - Get Attributes
+ */
+static void nfs4_xdr_enc_cb_getattr(struct rpc_rqst *req,
+ struct xdr_stream *xdr, const void *data)
+{
+ const struct nfsd4_callback *cb = data;
+ struct nfs4_cb_fattr *ncf =
+ container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
+ struct nfs4_delegation *dp =
+ container_of(ncf, struct nfs4_delegation, dl_cb_fattr);
+ struct nfs4_cb_compound_hdr hdr = {
+ .ident = cb->cb_clp->cl_cb_ident,
+ .minorversion = cb->cb_clp->cl_minorversion,
+ };
+
+ encode_cb_compound4args(xdr, &hdr);
+ encode_cb_sequence4args(xdr, cb, &hdr);
+ encode_cb_getattr4args(xdr, &hdr,
+ &dp->dl_stid.sc_file->fi_fhandle, &dp->dl_cb_fattr);
+ encode_cb_nops(&hdr);
+}
+
+/*
* 20.2. Operation 4: CB_RECALL - Recall a Delegation
*/
static void nfs4_xdr_enc_cb_recall(struct rpc_rqst *req, struct xdr_stream *xdr,
@@ -548,6 +628,42 @@ static int nfs4_xdr_dec_cb_null(struct rpc_rqst *req, struct xdr_stream *xdr,
}
/*
+ * 20.1. Operation 3: CB_GETATTR - Get Attributes
+ */
+static int nfs4_xdr_dec_cb_getattr(struct rpc_rqst *rqstp,
+ struct xdr_stream *xdr,
+ void *data)
+{
+ struct nfsd4_callback *cb = data;
+ struct nfs4_cb_compound_hdr hdr;
+ int status;
+ u32 bitmap[3] = {0};
+ u32 attrlen;
+ struct nfs4_cb_fattr *ncf =
+ container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
+
+ status = decode_cb_compound4res(xdr, &hdr);
+ if (unlikely(status))
+ return status;
+
+ status = decode_cb_sequence4res(xdr, cb);
+ if (unlikely(status || cb->cb_seq_status))
+ return status;
+
+ status = decode_cb_op_status(xdr, OP_CB_GETATTR, &cb->cb_status);
+ if (status)
+ return status;
+ decode_bitmap4(xdr, bitmap, 3);
+ status = decode_attr_length(xdr, &attrlen);
+ if (status)
+ return status;
+ ncf->ncf_cb_cinfo = 0;
+ ncf->ncf_cb_fsize = 0;
+ status = decode_cb_getattr(xdr, bitmap, ncf);
+ return status;
+}
+
+/*
* 20.2. Operation 4: CB_RECALL - Recall a Delegation
*/
static int nfs4_xdr_dec_cb_recall(struct rpc_rqst *rqstp,
@@ -855,6 +971,7 @@ static const struct rpc_procinfo nfs4_cb_procedures[] = {
PROC(CB_NOTIFY_LOCK, COMPOUND, cb_notify_lock, cb_notify_lock),
PROC(CB_OFFLOAD, COMPOUND, cb_offload, cb_offload),
PROC(CB_RECALL_ANY, COMPOUND, cb_recall_any, cb_recall_any),
+ PROC(CB_GETATTR, COMPOUND, cb_getattr, cb_getattr),
};
static unsigned int nfs4_cb_counts[ARRAY_SIZE(nfs4_cb_procedures)];
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index d49d3060ed4f..92349375053a 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -117,6 +117,19 @@ struct nfs4_cpntf_state {
time64_t cpntf_time; /* last time stateid used */
};
+struct nfs4_cb_fattr {
+ struct nfsd4_callback ncf_getattr;
+ u32 ncf_cb_status;
+ u32 ncf_cb_bmap[1];
+
+ /* from CB_GETATTR reply */
+ u64 ncf_cb_cinfo;
+ u64 ncf_cb_fsize;
+};
+
+/* bits for ncf_cb_flags */
+#define CB_GETATTR_BUSY 0
+
/*
* Represents a delegation stateid. The nfs4_client holds references to these
* and they are put when it is being destroyed or when the delegation is
@@ -150,6 +163,9 @@ struct nfs4_delegation {
int dl_retries;
struct nfsd4_callback dl_recall;
bool dl_recalled;
+
+ /* for CB_GETATTR */
+ struct nfs4_cb_fattr dl_cb_fattr;
};
#define cb_to_delegation(cb) \
@@ -642,6 +658,7 @@ enum nfsd4_cb_op {
NFSPROC4_CLNT_CB_SEQUENCE,
NFSPROC4_CLNT_CB_NOTIFY_LOCK,
NFSPROC4_CLNT_CB_RECALL_ANY,
+ NFSPROC4_CLNT_CB_GETATTR,
};
/* Returns true iff a is later than b: */
diff --git a/fs/nfsd/xdr4cb.h b/fs/nfsd/xdr4cb.h
index 0d39af1b00a0..3a31bb0a3ded 100644
--- a/fs/nfsd/xdr4cb.h
+++ b/fs/nfsd/xdr4cb.h
@@ -54,3 +54,22 @@
#define NFS4_dec_cb_recall_any_sz (cb_compound_dec_hdr_sz + \
cb_sequence_dec_sz + \
op_dec_sz)
+
+/*
+ * 1: CB_GETATTR opcode (32-bit)
+ * N: file_handle
+ * 1: number of entry in attribute array (32-bit)
+ * 1: entry 0 in attribute array (32-bit)
+ */
+#define NFS4_enc_cb_getattr_sz (cb_compound_enc_hdr_sz + \
+ cb_sequence_enc_sz + \
+ 1 + enc_nfs4_fh_sz + 1 + 1)
+/*
+ * 1: attr mask (32-bit bmap)
+ * 2: length of attribute array (64-bit)
+ * 2: change attr (64-bit)
+ * 2: size (64-bit)
+ */
+#define NFS4_dec_cb_getattr_sz (cb_compound_dec_hdr_sz + \
+ cb_sequence_dec_sz + 7 + \
+ op_dec_sz)
--
2.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 3/4] NFSD: add supports for CB_GETATTR callback
2023-05-11 21:43 ` [PATCH 3/4] NFSD: add supports for CB_GETATTR callback Dai Ngo
@ 2023-05-12 15:30 ` Chuck Lever III
2023-05-12 17:41 ` dai.ngo
0 siblings, 1 reply; 8+ messages in thread
From: Chuck Lever III @ 2023-05-12 15:30 UTC (permalink / raw)
To: Dai Ngo
Cc: jlayton@kernel.org, Linux NFS Mailing List,
linux-fsdevel@vger.kernel.org
Hey Dai-
Jeff's a little better with the state-related code, so let
me start with a review of the new CB_GETATTR implementation.
> On May 11, 2023, at 2:43 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>
> Includes:
> . CB_GETATTR proc for nfs4_cb_procedures[]
>
> . XDR encoding and decoding function for CB_GETATTR request/reply
>
> . add nfs4_cb_fattr to nfs4_delegation for sending CB_GETATTR
> and store file attributes from client's reply.
>
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
> fs/nfsd/nfs4callback.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
> fs/nfsd/state.h | 17 +++++++
> fs/nfsd/xdr4cb.h | 19 ++++++++
> 3 files changed, 153 insertions(+)
>
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 4039ffcf90ba..ca3d72ef5fbc 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -87,6 +87,43 @@ static void encode_bitmap4(struct xdr_stream *xdr, const __u32 *bitmap,
> WARN_ON_ONCE(xdr_stream_encode_uint32_array(xdr, bitmap, len) < 0);
> }
>
> +static void decode_bitmap4(struct xdr_stream *xdr, __u32 *bitmap,
> + size_t len)
> +{
> + WARN_ON_ONCE(xdr_stream_decode_uint32_array(xdr, bitmap, len) < 0);
> +}
encode_bitmap4() hides the WARN_ON_ONCE.
However, for decoding, we actually want to get the result
of the decode, so let's get rid of decode_bitmap4() and
simply call xdr_stream_decode_uint32_array() directly from
nfs4_xdr_dec_cb_getattr() (and, of course, check it's return
code properly, no WARN_ON).
> +
> +static int decode_attr_length(struct xdr_stream *xdr, uint32_t *attrlen)
> +{
> + __be32 *p;
> +
> + p = xdr_inline_decode(xdr, 4);
> + if (unlikely(!p))
> + return -EIO;
> + *attrlen = be32_to_cpup(p);
> + return 0;
> +}
> +
> +static int decode_cb_getattr(struct xdr_stream *xdr, uint32_t *bitmap,
> + struct nfs4_cb_fattr *fattr)
> +{
> + __be32 *ptr;
> +
> + if (likely(bitmap[0] & FATTR4_WORD0_CHANGE)) {
> + ptr = xdr_inline_decode(xdr, 8);
> + if (unlikely(!ptr))
> + return -EIO;
> + xdr_decode_hyper(ptr, &fattr->ncf_cb_cinfo);
> + }
> + if (likely(bitmap[0] & FATTR4_WORD0_SIZE)) {
> + ptr = xdr_inline_decode(xdr, 8);
> + if (unlikely(!ptr))
> + return -EIO;
> + xdr_decode_hyper(ptr, &fattr->ncf_cb_fsize);
> + }
Let's use xdr_stream_decode_u64() for these.
Also, I don't think the likely() is necessary -- this
isn't performance-sensitive code.
> + return 0;
> +}
> +
> /*
> * nfs_cb_opnum4
> *
> @@ -358,6 +395,26 @@ encode_cb_recallany4args(struct xdr_stream *xdr,
> }
>
> /*
> + * CB_GETATTR4args
> + * struct CB_GETATTR4args {
> + * nfs_fh4 fh;
> + * bitmap4 attr_request;
> + * };
> + *
> + * The size and change attributes are the only one
> + * guaranteed to be serviced by the client.
> + */
> +static void
> +encode_cb_getattr4args(struct xdr_stream *xdr, struct nfs4_cb_compound_hdr *hdr,
> + struct knfsd_fh *fh, struct nfs4_cb_fattr *fattr)
Nit: Can this take just a "struct nfs4_cb_fattr *" parameter
instead of the filehandle and fattr?
> +{
> + encode_nfs_cb_opnum4(xdr, OP_CB_GETATTR);
> + encode_nfs_fh4(xdr, fh);
> + encode_bitmap4(xdr, fattr->ncf_cb_bmap, ARRAY_SIZE(fattr->ncf_cb_bmap));
> + hdr->nops++;
> +}
> +
> +/*
> * CB_SEQUENCE4args
> *
> * struct CB_SEQUENCE4args {
> @@ -493,6 +550,29 @@ static void nfs4_xdr_enc_cb_null(struct rpc_rqst *req, struct xdr_stream *xdr,
> }
>
> /*
> + * 20.1. Operation 3: CB_GETATTR - Get Attributes
> + */
> +static void nfs4_xdr_enc_cb_getattr(struct rpc_rqst *req,
> + struct xdr_stream *xdr, const void *data)
> +{
> + const struct nfsd4_callback *cb = data;
> + struct nfs4_cb_fattr *ncf =
> + container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
> + struct nfs4_delegation *dp =
> + container_of(ncf, struct nfs4_delegation, dl_cb_fattr);
> + struct nfs4_cb_compound_hdr hdr = {
> + .ident = cb->cb_clp->cl_cb_ident,
> + .minorversion = cb->cb_clp->cl_minorversion,
> + };
> +
> + encode_cb_compound4args(xdr, &hdr);
> + encode_cb_sequence4args(xdr, cb, &hdr);
> + encode_cb_getattr4args(xdr, &hdr,
> + &dp->dl_stid.sc_file->fi_fhandle, &dp->dl_cb_fattr);
> + encode_cb_nops(&hdr);
> +}
> +
> +/*
> * 20.2. Operation 4: CB_RECALL - Recall a Delegation
> */
> static void nfs4_xdr_enc_cb_recall(struct rpc_rqst *req, struct xdr_stream *xdr,
> @@ -548,6 +628,42 @@ static int nfs4_xdr_dec_cb_null(struct rpc_rqst *req, struct xdr_stream *xdr,
> }
>
> /*
> + * 20.1. Operation 3: CB_GETATTR - Get Attributes
> + */
> +static int nfs4_xdr_dec_cb_getattr(struct rpc_rqst *rqstp,
> + struct xdr_stream *xdr,
> + void *data)
> +{
> + struct nfsd4_callback *cb = data;
> + struct nfs4_cb_compound_hdr hdr;
> + int status;
> + u32 bitmap[3] = {0};
> + u32 attrlen;
> + struct nfs4_cb_fattr *ncf =
> + container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
> +
> + status = decode_cb_compound4res(xdr, &hdr);
> + if (unlikely(status))
> + return status;
> +
> + status = decode_cb_sequence4res(xdr, cb);
> + if (unlikely(status || cb->cb_seq_status))
> + return status;
> +
> + status = decode_cb_op_status(xdr, OP_CB_GETATTR, &cb->cb_status);
> + if (status)
> + return status;
> + decode_bitmap4(xdr, bitmap, 3);
> + status = decode_attr_length(xdr, &attrlen);
> + if (status)
> + return status;
Let's use xdr_stream_decode_u32 directly here, and
check that attrlen is a reasonable value. Say, not
larger than the full expected array length?
> + ncf->ncf_cb_cinfo = 0;
> + ncf->ncf_cb_fsize = 0;
> + status = decode_cb_getattr(xdr, bitmap, ncf);
> + return status;
You're actually decoding a fattr4 here, not the whole
CB_GETATTR result. Can we call the function
decode_cb_fattr4() ?
Nit: Let's fold the initialization of cb_cinfo and
cb_fsize into decode_cb_fattr4().
> +}
> +
> +/*
> * 20.2. Operation 4: CB_RECALL - Recall a Delegation
> */
> static int nfs4_xdr_dec_cb_recall(struct rpc_rqst *rqstp,
> @@ -855,6 +971,7 @@ static const struct rpc_procinfo nfs4_cb_procedures[] = {
> PROC(CB_NOTIFY_LOCK, COMPOUND, cb_notify_lock, cb_notify_lock),
> PROC(CB_OFFLOAD, COMPOUND, cb_offload, cb_offload),
> PROC(CB_RECALL_ANY, COMPOUND, cb_recall_any, cb_recall_any),
> + PROC(CB_GETATTR, COMPOUND, cb_getattr, cb_getattr),
> };
>
> static unsigned int nfs4_cb_counts[ARRAY_SIZE(nfs4_cb_procedures)];
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index d49d3060ed4f..92349375053a 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -117,6 +117,19 @@ struct nfs4_cpntf_state {
> time64_t cpntf_time; /* last time stateid used */
> };
>
> +struct nfs4_cb_fattr {
> + struct nfsd4_callback ncf_getattr;
> + u32 ncf_cb_status;
> + u32 ncf_cb_bmap[1];
> +
> + /* from CB_GETATTR reply */
> + u64 ncf_cb_cinfo;
In fs/nfsd/nfs4xdr.c, we use "cinfo" to mean change info:
that's a boolean, and a pair of 64-bit change attribute values.
IIUC, I don't think that's what this is; it's just a single
change attribute value.
To avoid overloading the name "cinfo" can you call this field
something like ncf_cb_change ?
> + u64 ncf_cb_fsize;
> +};
> +
> +/* bits for ncf_cb_flags */
> +#define CB_GETATTR_BUSY 0
> +
> /*
> * Represents a delegation stateid. The nfs4_client holds references to these
> * and they are put when it is being destroyed or when the delegation is
> @@ -150,6 +163,9 @@ struct nfs4_delegation {
> int dl_retries;
> struct nfsd4_callback dl_recall;
> bool dl_recalled;
> +
> + /* for CB_GETATTR */
> + struct nfs4_cb_fattr dl_cb_fattr;
> };
>
> #define cb_to_delegation(cb) \
> @@ -642,6 +658,7 @@ enum nfsd4_cb_op {
> NFSPROC4_CLNT_CB_SEQUENCE,
> NFSPROC4_CLNT_CB_NOTIFY_LOCK,
> NFSPROC4_CLNT_CB_RECALL_ANY,
> + NFSPROC4_CLNT_CB_GETATTR,
> };
>
> /* Returns true iff a is later than b: */
> diff --git a/fs/nfsd/xdr4cb.h b/fs/nfsd/xdr4cb.h
> index 0d39af1b00a0..3a31bb0a3ded 100644
> --- a/fs/nfsd/xdr4cb.h
> +++ b/fs/nfsd/xdr4cb.h
> @@ -54,3 +54,22 @@
> #define NFS4_dec_cb_recall_any_sz (cb_compound_dec_hdr_sz + \
> cb_sequence_dec_sz + \
> op_dec_sz)
> +
> +/*
> + * 1: CB_GETATTR opcode (32-bit)
> + * N: file_handle
> + * 1: number of entry in attribute array (32-bit)
> + * 1: entry 0 in attribute array (32-bit)
> + */
> +#define NFS4_enc_cb_getattr_sz (cb_compound_enc_hdr_sz + \
> + cb_sequence_enc_sz + \
> + 1 + enc_nfs4_fh_sz + 1 + 1)
> +/*
> + * 1: attr mask (32-bit bmap)
> + * 2: length of attribute array (64-bit)
Isn't the array length field 32 bits?
> + * 2: change attr (64-bit)
> + * 2: size (64-bit)
> + */
> +#define NFS4_dec_cb_getattr_sz (cb_compound_dec_hdr_sz + \
> + cb_sequence_dec_sz + 7 + \
Generally we list the length of each item separately
to document the individual values, so:
1 + 1 + 2 + 2
is preferred over a single integer.
> + op_dec_sz)
> --
> 2.9.5
>
--
Chuck Lever
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 3/4] NFSD: add supports for CB_GETATTR callback
2023-05-12 15:30 ` Chuck Lever III
@ 2023-05-12 17:41 ` dai.ngo
0 siblings, 0 replies; 8+ messages in thread
From: dai.ngo @ 2023-05-12 17:41 UTC (permalink / raw)
To: Chuck Lever III
Cc: jlayton@kernel.org, Linux NFS Mailing List,
linux-fsdevel@vger.kernel.org
Thank you Chuck for your review. I'll make the change in v2.
-Dai
On 5/12/23 8:30 AM, Chuck Lever III wrote:
> Hey Dai-
>
> Jeff's a little better with the state-related code, so let
> me start with a review of the new CB_GETATTR implementation.
>
>
>> On May 11, 2023, at 2:43 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>
>> Includes:
>> . CB_GETATTR proc for nfs4_cb_procedures[]
>>
>> . XDR encoding and decoding function for CB_GETATTR request/reply
>>
>> . add nfs4_cb_fattr to nfs4_delegation for sending CB_GETATTR
>> and store file attributes from client's reply.
>>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>> fs/nfsd/nfs4callback.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
>> fs/nfsd/state.h | 17 +++++++
>> fs/nfsd/xdr4cb.h | 19 ++++++++
>> 3 files changed, 153 insertions(+)
>>
>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
>> index 4039ffcf90ba..ca3d72ef5fbc 100644
>> --- a/fs/nfsd/nfs4callback.c
>> +++ b/fs/nfsd/nfs4callback.c
>> @@ -87,6 +87,43 @@ static void encode_bitmap4(struct xdr_stream *xdr, const __u32 *bitmap,
>> WARN_ON_ONCE(xdr_stream_encode_uint32_array(xdr, bitmap, len) < 0);
>> }
>>
>> +static void decode_bitmap4(struct xdr_stream *xdr, __u32 *bitmap,
>> + size_t len)
>> +{
>> + WARN_ON_ONCE(xdr_stream_decode_uint32_array(xdr, bitmap, len) < 0);
>> +}
> encode_bitmap4() hides the WARN_ON_ONCE.
>
> However, for decoding, we actually want to get the result
> of the decode, so let's get rid of decode_bitmap4() and
> simply call xdr_stream_decode_uint32_array() directly from
> nfs4_xdr_dec_cb_getattr() (and, of course, check it's return
> code properly, no WARN_ON).
>
>
>> +
>> +static int decode_attr_length(struct xdr_stream *xdr, uint32_t *attrlen)
>> +{
>> + __be32 *p;
>> +
>> + p = xdr_inline_decode(xdr, 4);
>> + if (unlikely(!p))
>> + return -EIO;
>> + *attrlen = be32_to_cpup(p);
>> + return 0;
>> +}
>> +
>> +static int decode_cb_getattr(struct xdr_stream *xdr, uint32_t *bitmap,
>> + struct nfs4_cb_fattr *fattr)
>> +{
>> + __be32 *ptr;
>> +
>> + if (likely(bitmap[0] & FATTR4_WORD0_CHANGE)) {
>> + ptr = xdr_inline_decode(xdr, 8);
>> + if (unlikely(!ptr))
>> + return -EIO;
>> + xdr_decode_hyper(ptr, &fattr->ncf_cb_cinfo);
>> + }
>> + if (likely(bitmap[0] & FATTR4_WORD0_SIZE)) {
>> + ptr = xdr_inline_decode(xdr, 8);
>> + if (unlikely(!ptr))
>> + return -EIO;
>> + xdr_decode_hyper(ptr, &fattr->ncf_cb_fsize);
>> + }
> Let's use xdr_stream_decode_u64() for these.
>
> Also, I don't think the likely() is necessary -- this
> isn't performance-sensitive code.
>
>
>> + return 0;
>> +}
>> +
>> /*
>> * nfs_cb_opnum4
>> *
>> @@ -358,6 +395,26 @@ encode_cb_recallany4args(struct xdr_stream *xdr,
>> }
>>
>> /*
>> + * CB_GETATTR4args
>> + * struct CB_GETATTR4args {
>> + * nfs_fh4 fh;
>> + * bitmap4 attr_request;
>> + * };
>> + *
>> + * The size and change attributes are the only one
>> + * guaranteed to be serviced by the client.
>> + */
>> +static void
>> +encode_cb_getattr4args(struct xdr_stream *xdr, struct nfs4_cb_compound_hdr *hdr,
>> + struct knfsd_fh *fh, struct nfs4_cb_fattr *fattr)
> Nit: Can this take just a "struct nfs4_cb_fattr *" parameter
> instead of the filehandle and fattr?
>
>
>> +{
>> + encode_nfs_cb_opnum4(xdr, OP_CB_GETATTR);
>> + encode_nfs_fh4(xdr, fh);
>> + encode_bitmap4(xdr, fattr->ncf_cb_bmap, ARRAY_SIZE(fattr->ncf_cb_bmap));
>> + hdr->nops++;
>> +}
>> +
>> +/*
>> * CB_SEQUENCE4args
>> *
>> * struct CB_SEQUENCE4args {
>> @@ -493,6 +550,29 @@ static void nfs4_xdr_enc_cb_null(struct rpc_rqst *req, struct xdr_stream *xdr,
>> }
>>
>> /*
>> + * 20.1. Operation 3: CB_GETATTR - Get Attributes
>> + */
>> +static void nfs4_xdr_enc_cb_getattr(struct rpc_rqst *req,
>> + struct xdr_stream *xdr, const void *data)
>> +{
>> + const struct nfsd4_callback *cb = data;
>> + struct nfs4_cb_fattr *ncf =
>> + container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
>> + struct nfs4_delegation *dp =
>> + container_of(ncf, struct nfs4_delegation, dl_cb_fattr);
>> + struct nfs4_cb_compound_hdr hdr = {
>> + .ident = cb->cb_clp->cl_cb_ident,
>> + .minorversion = cb->cb_clp->cl_minorversion,
>> + };
>> +
>> + encode_cb_compound4args(xdr, &hdr);
>> + encode_cb_sequence4args(xdr, cb, &hdr);
>> + encode_cb_getattr4args(xdr, &hdr,
>> + &dp->dl_stid.sc_file->fi_fhandle, &dp->dl_cb_fattr);
>> + encode_cb_nops(&hdr);
>> +}
>> +
>> +/*
>> * 20.2. Operation 4: CB_RECALL - Recall a Delegation
>> */
>> static void nfs4_xdr_enc_cb_recall(struct rpc_rqst *req, struct xdr_stream *xdr,
>> @@ -548,6 +628,42 @@ static int nfs4_xdr_dec_cb_null(struct rpc_rqst *req, struct xdr_stream *xdr,
>> }
>>
>> /*
>> + * 20.1. Operation 3: CB_GETATTR - Get Attributes
>> + */
>> +static int nfs4_xdr_dec_cb_getattr(struct rpc_rqst *rqstp,
>> + struct xdr_stream *xdr,
>> + void *data)
>> +{
>> + struct nfsd4_callback *cb = data;
>> + struct nfs4_cb_compound_hdr hdr;
>> + int status;
>> + u32 bitmap[3] = {0};
>> + u32 attrlen;
>> + struct nfs4_cb_fattr *ncf =
>> + container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
>> +
>> + status = decode_cb_compound4res(xdr, &hdr);
>> + if (unlikely(status))
>> + return status;
>> +
>> + status = decode_cb_sequence4res(xdr, cb);
>> + if (unlikely(status || cb->cb_seq_status))
>> + return status;
>> +
>> + status = decode_cb_op_status(xdr, OP_CB_GETATTR, &cb->cb_status);
>> + if (status)
>> + return status;
>> + decode_bitmap4(xdr, bitmap, 3);
>> + status = decode_attr_length(xdr, &attrlen);
>> + if (status)
>> + return status;
> Let's use xdr_stream_decode_u32 directly here, and
> check that attrlen is a reasonable value. Say, not
> larger than the full expected array length?
>
>
>> + ncf->ncf_cb_cinfo = 0;
>> + ncf->ncf_cb_fsize = 0;
>> + status = decode_cb_getattr(xdr, bitmap, ncf);
>> + return status;
> You're actually decoding a fattr4 here, not the whole
> CB_GETATTR result. Can we call the function
> decode_cb_fattr4() ?
>
> Nit: Let's fold the initialization of cb_cinfo and
> cb_fsize into decode_cb_fattr4().
>
>
>
>> +}
>> +
>> +/*
>> * 20.2. Operation 4: CB_RECALL - Recall a Delegation
>> */
>> static int nfs4_xdr_dec_cb_recall(struct rpc_rqst *rqstp,
>> @@ -855,6 +971,7 @@ static const struct rpc_procinfo nfs4_cb_procedures[] = {
>> PROC(CB_NOTIFY_LOCK, COMPOUND, cb_notify_lock, cb_notify_lock),
>> PROC(CB_OFFLOAD, COMPOUND, cb_offload, cb_offload),
>> PROC(CB_RECALL_ANY, COMPOUND, cb_recall_any, cb_recall_any),
>> + PROC(CB_GETATTR, COMPOUND, cb_getattr, cb_getattr),
>> };
>>
>> static unsigned int nfs4_cb_counts[ARRAY_SIZE(nfs4_cb_procedures)];
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index d49d3060ed4f..92349375053a 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -117,6 +117,19 @@ struct nfs4_cpntf_state {
>> time64_t cpntf_time; /* last time stateid used */
>> };
>>
>> +struct nfs4_cb_fattr {
>> + struct nfsd4_callback ncf_getattr;
>> + u32 ncf_cb_status;
>> + u32 ncf_cb_bmap[1];
>> +
>> + /* from CB_GETATTR reply */
>> + u64 ncf_cb_cinfo;
> In fs/nfsd/nfs4xdr.c, we use "cinfo" to mean change info:
> that's a boolean, and a pair of 64-bit change attribute values.
> IIUC, I don't think that's what this is; it's just a single
> change attribute value.
>
> To avoid overloading the name "cinfo" can you call this field
> something like ncf_cb_change ?
>
>
>> + u64 ncf_cb_fsize;
>> +};
>> +
>> +/* bits for ncf_cb_flags */
>> +#define CB_GETATTR_BUSY 0
>> +
>> /*
>> * Represents a delegation stateid. The nfs4_client holds references to these
>> * and they are put when it is being destroyed or when the delegation is
>> @@ -150,6 +163,9 @@ struct nfs4_delegation {
>> int dl_retries;
>> struct nfsd4_callback dl_recall;
>> bool dl_recalled;
>> +
>> + /* for CB_GETATTR */
>> + struct nfs4_cb_fattr dl_cb_fattr;
>> };
>>
>> #define cb_to_delegation(cb) \
>> @@ -642,6 +658,7 @@ enum nfsd4_cb_op {
>> NFSPROC4_CLNT_CB_SEQUENCE,
>> NFSPROC4_CLNT_CB_NOTIFY_LOCK,
>> NFSPROC4_CLNT_CB_RECALL_ANY,
>> + NFSPROC4_CLNT_CB_GETATTR,
>> };
>>
>> /* Returns true iff a is later than b: */
>> diff --git a/fs/nfsd/xdr4cb.h b/fs/nfsd/xdr4cb.h
>> index 0d39af1b00a0..3a31bb0a3ded 100644
>> --- a/fs/nfsd/xdr4cb.h
>> +++ b/fs/nfsd/xdr4cb.h
>> @@ -54,3 +54,22 @@
>> #define NFS4_dec_cb_recall_any_sz (cb_compound_dec_hdr_sz + \
>> cb_sequence_dec_sz + \
>> op_dec_sz)
>> +
>> +/*
>> + * 1: CB_GETATTR opcode (32-bit)
>> + * N: file_handle
>> + * 1: number of entry in attribute array (32-bit)
>> + * 1: entry 0 in attribute array (32-bit)
>> + */
>> +#define NFS4_enc_cb_getattr_sz (cb_compound_enc_hdr_sz + \
>> + cb_sequence_enc_sz + \
>> + 1 + enc_nfs4_fh_sz + 1 + 1)
>> +/*
>> + * 1: attr mask (32-bit bmap)
>> + * 2: length of attribute array (64-bit)
> Isn't the array length field 32 bits?
>
>
>> + * 2: change attr (64-bit)
>> + * 2: size (64-bit)
>> + */
>> +#define NFS4_dec_cb_getattr_sz (cb_compound_dec_hdr_sz + \
>> + cb_sequence_dec_sz + 7 + \
> Generally we list the length of each item separately
> to document the individual values, so:
>
> 1 + 1 + 2 + 2
>
> is preferred over a single integer.
>
>
>> + op_dec_sz)
>> --
>> 2.9.5
>>
> --
> Chuck Lever
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
2023-05-11 21:42 [PATCH 0/4] NFSD: add support for NFSv4 write delegation Dai Ngo
` (2 preceding siblings ...)
2023-05-11 21:43 ` [PATCH 3/4] NFSD: add supports for CB_GETATTR callback Dai Ngo
@ 2023-05-11 21:43 ` Dai Ngo
2023-05-12 14:05 ` kernel test robot
3 siblings, 1 reply; 8+ messages in thread
From: Dai Ngo @ 2023-05-11 21:43 UTC (permalink / raw)
To: chuck.lever, jlayton; +Cc: linux-nfs, linux-fsdevel
If the GETATTR request on a file that has write delegation in effect
and the request attributes include the change info and size attribute
then the request is handled as below:
Server sends CB_GETATTR to client to get the latest change info and file
size. If these values are the same as the server's cached values then
the GETATTR proceeds as normal.
If either the change info or file size is different from the server's
cached value, or the file was already marked as modified, then:
. update time_modify and time_metadata in the file's metadata
with current time
. encode GETATTR as normal except the file size is encoded with
the value returned from the CB_GETATTR
. mark the file as modified
If the CB_GETATTR fails for any reasons, the delegation is recalled
and NFS4ERR_DELAY is returned for the GETATTR.
Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
fs/nfsd/nfs4state.c | 58 ++++++++++++++++++++++++++++++++++++
fs/nfsd/nfs4xdr.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
fs/nfsd/state.h | 7 +++++
3 files changed, 148 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 09a9e16407f9..fb305b28a090 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -127,6 +127,7 @@ static void free_session(struct nfsd4_session *);
static const struct nfsd4_callback_ops nfsd4_cb_recall_ops;
static const struct nfsd4_callback_ops nfsd4_cb_notify_lock_ops;
+static const struct nfsd4_callback_ops nfsd4_cb_getattr_ops;
static struct workqueue_struct *laundry_wq;
@@ -1175,6 +1176,10 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
dp->dl_recalled = false;
nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
&nfsd4_cb_recall_ops, NFSPROC4_CLNT_CB_RECALL);
+ nfsd4_init_cb(&dp->dl_cb_fattr.ncf_getattr, dp->dl_stid.sc_client,
+ &nfsd4_cb_getattr_ops, NFSPROC4_CLNT_CB_GETATTR);
+ dp->dl_cb_fattr.ncf_file_modified = false;
+ dp->dl_cb_fattr.ncf_cb_bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
get_nfs4_file(fp);
dp->dl_stid.sc_file = fp;
return dp;
@@ -2882,11 +2887,49 @@ nfsd4_cb_recall_any_release(struct nfsd4_callback *cb)
spin_unlock(&nn->client_lock);
}
+static int
+nfsd4_cb_getattr_done(struct nfsd4_callback *cb, struct rpc_task *task)
+{
+ struct nfs4_cb_fattr *ncf =
+ container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
+
+ ncf->ncf_cb_status = task->tk_status;
+ switch (task->tk_status) {
+ case -NFS4ERR_DELAY:
+ rpc_delay(task, 2 * HZ);
+ return 0;
+ default:
+ return 1;
+ }
+}
+
+static void
+nfsd4_cb_getattr_release(struct nfsd4_callback *cb)
+{
+ struct nfs4_cb_fattr *ncf =
+ container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
+
+ clear_bit(CB_GETATTR_BUSY, &ncf->ncf_cb_flags);
+ wake_up_bit(&ncf->ncf_cb_flags, CB_GETATTR_BUSY);
+}
+
static const struct nfsd4_callback_ops nfsd4_cb_recall_any_ops = {
.done = nfsd4_cb_recall_any_done,
.release = nfsd4_cb_recall_any_release,
};
+static const struct nfsd4_callback_ops nfsd4_cb_getattr_ops = {
+ .done = nfsd4_cb_getattr_done,
+ .release = nfsd4_cb_getattr_release,
+};
+
+void nfs4_cb_getattr(struct nfs4_cb_fattr *ncf)
+{
+ if (test_and_set_bit(CB_GETATTR_BUSY, &ncf->ncf_cb_flags))
+ return;
+ nfsd4_run_cb(&ncf->ncf_getattr);
+}
+
static struct nfs4_client *create_client(struct xdr_netobj name,
struct svc_rqst *rqstp, nfs4_verifier *verf)
{
@@ -5591,6 +5634,8 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
int cb_up;
int status = 0;
u32 wdeleg = false;
+ struct kstat stat;
+ struct path path;
cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client);
open->op_recall = 0;
@@ -5626,6 +5671,19 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
wdeleg = open->op_share_access & NFS4_SHARE_ACCESS_WRITE;
open->op_delegate_type = wdeleg ?
NFS4_OPEN_DELEGATE_WRITE : NFS4_OPEN_DELEGATE_READ;
+ if (wdeleg) {
+ path.mnt = currentfh->fh_export->ex_path.mnt;
+ path.dentry = currentfh->fh_dentry;
+ if (vfs_getattr(&path, &stat, STATX_BASIC_STATS,
+ AT_STATX_SYNC_AS_STAT)) {
+ nfs4_put_stid(&dp->dl_stid);
+ destroy_delegation(dp);
+ goto out_no_deleg;
+ }
+ dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
+ dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat,
+ d_inode(currentfh->fh_dentry));
+ }
nfs4_put_stid(&dp->dl_stid);
return;
out_no_deleg:
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 76db2fe29624..52d51784509e 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2920,6 +2920,77 @@ nfsd4_encode_bitmap(struct xdr_stream *xdr, u32 bmval0, u32 bmval1, u32 bmval2)
return nfserr_resource;
}
+static struct file_lock *
+nfs4_wrdeleg_filelock(struct svc_rqst *rqstp, struct inode *inode)
+{
+ struct file_lock_context *ctx;
+ struct file_lock *fl;
+
+ ctx = locks_inode_context(inode);
+ if (!ctx)
+ return NULL;
+ spin_lock(&ctx->flc_lock);
+ list_for_each_entry(fl, &ctx->flc_lease, fl_list) {
+ if (fl->fl_type == F_WRLCK) {
+ spin_unlock(&ctx->flc_lock);
+ return fl;
+ }
+ }
+ spin_unlock(&ctx->flc_lock);
+ return NULL;
+}
+
+static int
+nfs4_handle_wrdeleg_conflict(struct svc_rqst *rqstp, struct inode *inode,
+ bool *modified, u64 *size)
+{
+ __be32 status;
+ struct file_lock *fl;
+ struct nfs4_delegation *dp;
+ struct nfs4_cb_fattr *ncf;
+ struct iattr attrs;
+
+ *modified = false;
+ fl = nfs4_wrdeleg_filelock(rqstp, inode);
+ if (!fl)
+ return 0;
+ dp = fl->fl_owner;
+ ncf = &dp->dl_cb_fattr;
+ if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker))
+ return 0;
+
+ refcount_inc(&dp->dl_stid.sc_count);
+ nfs4_cb_getattr(&dp->dl_cb_fattr);
+ wait_on_bit(&ncf->ncf_cb_flags, CB_GETATTR_BUSY, TASK_INTERRUPTIBLE);
+ if (ncf->ncf_cb_status) {
+ status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
+ nfs4_put_stid(&dp->dl_stid);
+ return status;
+ }
+ ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
+ if (!ncf->ncf_file_modified &&
+ (ncf->ncf_initial_cinfo != ncf->ncf_cb_cinfo ||
+ ncf->ncf_cur_fsize != ncf->ncf_cb_fsize)) {
+ ncf->ncf_file_modified = true;
+ }
+
+ if (ncf->ncf_file_modified) {
+ /*
+ * The server would not update the file's metadata
+ * with the client's modified size.
+ * nfsd4 change attribute is constructed from ctime.
+ */
+ attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
+ attrs.ia_valid = ATTR_MTIME | ATTR_CTIME;
+ setattr_copy(&nop_mnt_idmap, inode, &attrs);
+ mark_inode_dirty(inode);
+ *size = ncf->ncf_cur_fsize;
+ *modified = true;
+ }
+ nfs4_put_stid(&dp->dl_stid);
+ return 0;
+}
+
/*
* Note: @fhp can be NULL; in this case, we might have to compose the filehandle
* ourselves.
@@ -2957,6 +3028,8 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
.dentry = dentry,
};
struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
+ bool file_modified;
+ u64 size = 0;
BUG_ON(bmval1 & NFSD_WRITEONLY_ATTRS_WORD1);
BUG_ON(!nfsd_attrs_supported(minorversion, bmval));
@@ -2966,6 +3039,12 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
if (status)
goto out;
}
+ if (bmval0 & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
+ status = nfs4_handle_wrdeleg_conflict(rqstp, d_inode(dentry),
+ &file_modified, &size);
+ if (status)
+ goto out;
+ }
err = vfs_getattr(&path, &stat,
STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
@@ -3089,7 +3168,10 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
p = xdr_reserve_space(xdr, 8);
if (!p)
goto out_resource;
- p = xdr_encode_hyper(p, stat.size);
+ if (file_modified)
+ p = xdr_encode_hyper(p, size);
+ else
+ p = xdr_encode_hyper(p, stat.size);
}
if (bmval0 & FATTR4_WORD0_LINK_SUPPORT) {
p = xdr_reserve_space(xdr, 4);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 92349375053a..cf49f26442e5 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -121,6 +121,10 @@ struct nfs4_cb_fattr {
struct nfsd4_callback ncf_getattr;
u32 ncf_cb_status;
u32 ncf_cb_bmap[1];
+ unsigned long ncf_cb_flags;
+ bool ncf_file_modified;
+ u64 ncf_initial_cinfo;
+ u64 ncf_cur_fsize;
/* from CB_GETATTR reply */
u64 ncf_cb_cinfo;
@@ -744,6 +748,9 @@ extern void nfsd4_client_record_remove(struct nfs4_client *clp);
extern int nfsd4_client_record_check(struct nfs4_client *clp);
extern void nfsd4_record_grace_done(struct nfsd_net *nn);
+/* CB_GETTTAR */
+extern void nfs4_cb_getattr(struct nfs4_cb_fattr *ncf);
+
static inline bool try_to_expire_client(struct nfs4_client *clp)
{
cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
--
2.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
2023-05-11 21:43 ` [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation Dai Ngo
@ 2023-05-12 14:05 ` kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2023-05-12 14:05 UTC (permalink / raw)
To: Dai Ngo, chuck.lever, jlayton; +Cc: oe-kbuild-all, linux-nfs, linux-fsdevel
Hi Dai,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.4-rc1 next-20230512]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dai-Ngo/locks-allow-support-for-write-delegation/20230512-054404
base: linus/master
patch link: https://lore.kernel.org/r/1683841383-21372-5-git-send-email-dai.ngo%40oracle.com
patch subject: [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
config: m68k-randconfig-s041-20230509 (https://download.01.org/0day-ci/archive/20230512/202305122100.rFiPDpBs-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/9e8fd28524572f2f87cc153cbaaaa7a4120d2319
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Dai-Ngo/locks-allow-support-for-write-delegation/20230512-054404
git checkout 9e8fd28524572f2f87cc153cbaaaa7a4120d2319
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=m68k SHELL=/bin/bash fs/nfsd/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305122100.rFiPDpBs-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> fs/nfsd/nfs4xdr.c:2968:24: sparse: sparse: incorrect type in return expression (different base types) @@ expected int @@ got restricted __be32 [assigned] [usertype] status @@
fs/nfsd/nfs4xdr.c:2968:24: sparse: expected int
fs/nfsd/nfs4xdr.c:2968:24: sparse: got restricted __be32 [assigned] [usertype] status
>> fs/nfsd/nfs4xdr.c:3043:24: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __be32 [assigned] [usertype] status @@ got int @@
fs/nfsd/nfs4xdr.c:3043:24: sparse: expected restricted __be32 [assigned] [usertype] status
fs/nfsd/nfs4xdr.c:3043:24: sparse: got int
vim +2968 fs/nfsd/nfs4xdr.c
2942
2943 static int
2944 nfs4_handle_wrdeleg_conflict(struct svc_rqst *rqstp, struct inode *inode,
2945 bool *modified, u64 *size)
2946 {
2947 __be32 status;
2948 struct file_lock *fl;
2949 struct nfs4_delegation *dp;
2950 struct nfs4_cb_fattr *ncf;
2951 struct iattr attrs;
2952
2953 *modified = false;
2954 fl = nfs4_wrdeleg_filelock(rqstp, inode);
2955 if (!fl)
2956 return 0;
2957 dp = fl->fl_owner;
2958 ncf = &dp->dl_cb_fattr;
2959 if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker))
2960 return 0;
2961
2962 refcount_inc(&dp->dl_stid.sc_count);
2963 nfs4_cb_getattr(&dp->dl_cb_fattr);
2964 wait_on_bit(&ncf->ncf_cb_flags, CB_GETATTR_BUSY, TASK_INTERRUPTIBLE);
2965 if (ncf->ncf_cb_status) {
2966 status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
2967 nfs4_put_stid(&dp->dl_stid);
> 2968 return status;
2969 }
2970 ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
2971 if (!ncf->ncf_file_modified &&
2972 (ncf->ncf_initial_cinfo != ncf->ncf_cb_cinfo ||
2973 ncf->ncf_cur_fsize != ncf->ncf_cb_fsize)) {
2974 ncf->ncf_file_modified = true;
2975 }
2976
2977 if (ncf->ncf_file_modified) {
2978 /*
2979 * The server would not update the file's metadata
2980 * with the client's modified size.
2981 * nfsd4 change attribute is constructed from ctime.
2982 */
2983 attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
2984 attrs.ia_valid = ATTR_MTIME | ATTR_CTIME;
2985 setattr_copy(&nop_mnt_idmap, inode, &attrs);
2986 mark_inode_dirty(inode);
2987 *size = ncf->ncf_cur_fsize;
2988 *modified = true;
2989 }
2990 nfs4_put_stid(&dp->dl_stid);
2991 return 0;
2992 }
2993
2994 /*
2995 * Note: @fhp can be NULL; in this case, we might have to compose the filehandle
2996 * ourselves.
2997 */
2998 static __be32
2999 nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
3000 struct svc_export *exp,
3001 struct dentry *dentry, u32 *bmval,
3002 struct svc_rqst *rqstp, int ignore_crossmnt)
3003 {
3004 u32 bmval0 = bmval[0];
3005 u32 bmval1 = bmval[1];
3006 u32 bmval2 = bmval[2];
3007 struct kstat stat;
3008 struct svc_fh *tempfh = NULL;
3009 struct kstatfs statfs;
3010 __be32 *p, *attrlen_p;
3011 int starting_len = xdr->buf->len;
3012 int attrlen_offset;
3013 u32 dummy;
3014 u64 dummy64;
3015 u32 rdattr_err = 0;
3016 __be32 status;
3017 int err;
3018 struct nfs4_acl *acl = NULL;
3019 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
3020 void *context = NULL;
3021 int contextlen;
3022 #endif
3023 bool contextsupport = false;
3024 struct nfsd4_compoundres *resp = rqstp->rq_resp;
3025 u32 minorversion = resp->cstate.minorversion;
3026 struct path path = {
3027 .mnt = exp->ex_path.mnt,
3028 .dentry = dentry,
3029 };
3030 struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
3031 bool file_modified;
3032 u64 size = 0;
3033
3034 BUG_ON(bmval1 & NFSD_WRITEONLY_ATTRS_WORD1);
3035 BUG_ON(!nfsd_attrs_supported(minorversion, bmval));
3036
3037 if (exp->ex_fslocs.migrated) {
3038 status = fattr_handle_absent_fs(&bmval0, &bmval1, &bmval2, &rdattr_err);
3039 if (status)
3040 goto out;
3041 }
3042 if (bmval0 & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
> 3043 status = nfs4_handle_wrdeleg_conflict(rqstp, d_inode(dentry),
3044 &file_modified, &size);
3045 if (status)
3046 goto out;
3047 }
3048
3049 err = vfs_getattr(&path, &stat,
3050 STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
3051 AT_STATX_SYNC_AS_STAT);
3052 if (err)
3053 goto out_nfserr;
3054 if (!(stat.result_mask & STATX_BTIME))
3055 /* underlying FS does not offer btime so we can't share it */
3056 bmval1 &= ~FATTR4_WORD1_TIME_CREATE;
3057 if ((bmval0 & (FATTR4_WORD0_FILES_AVAIL | FATTR4_WORD0_FILES_FREE |
3058 FATTR4_WORD0_FILES_TOTAL | FATTR4_WORD0_MAXNAME)) ||
3059 (bmval1 & (FATTR4_WORD1_SPACE_AVAIL | FATTR4_WORD1_SPACE_FREE |
3060 FATTR4_WORD1_SPACE_TOTAL))) {
3061 err = vfs_statfs(&path, &statfs);
3062 if (err)
3063 goto out_nfserr;
3064 }
3065 if ((bmval0 & (FATTR4_WORD0_FILEHANDLE | FATTR4_WORD0_FSID)) && !fhp) {
3066 tempfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
3067 status = nfserr_jukebox;
3068 if (!tempfh)
3069 goto out;
3070 fh_init(tempfh, NFS4_FHSIZE);
3071 status = fh_compose(tempfh, exp, dentry, NULL);
3072 if (status)
3073 goto out;
3074 fhp = tempfh;
3075 }
3076 if (bmval0 & FATTR4_WORD0_ACL) {
3077 err = nfsd4_get_nfs4_acl(rqstp, dentry, &acl);
3078 if (err == -EOPNOTSUPP)
3079 bmval0 &= ~FATTR4_WORD0_ACL;
3080 else if (err == -EINVAL) {
3081 status = nfserr_attrnotsupp;
3082 goto out;
3083 } else if (err != 0)
3084 goto out_nfserr;
3085 }
3086
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 8+ messages in thread