* [PATCH 6/6] NFS: discard nfs_lockowner structure.
2016-10-13 4:26 [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2 NeilBrown
@ 2016-10-13 4:26 ` NeilBrown
2016-10-13 4:26 ` [PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state NeilBrown
` (5 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: NeilBrown @ 2016-10-13 4:26 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, Jeff Layton
Cc: Benjamin Coddington, Linux NFS Mailing List
It now has only one field and is only used in one structure.
So replaced it in that structure by the field it contains.
Signed-off-by: NeilBrown <neilb@suse.com>
---
fs/nfs/inode.c | 4 ++--
fs/nfs/nfs4state.c | 2 +-
fs/nfs/pagelist.c | 2 +-
fs/nfs/write.c | 2 +-
include/linux/nfs_fs.h | 6 +-----
5 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 2138027eaba9..72e992f0f049 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -702,7 +702,7 @@ EXPORT_SYMBOL_GPL(nfs_getattr);
static void nfs_init_lock_context(struct nfs_lock_context *l_ctx)
{
atomic_set(&l_ctx->count, 1);
- l_ctx->lockowner.l_owner = current->files;
+ l_ctx->lockowner = current->files;
INIT_LIST_HEAD(&l_ctx->list);
atomic_set(&l_ctx->io_count, 0);
}
@@ -713,7 +713,7 @@ static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context
struct nfs_lock_context *pos = head;
do {
- if (pos->lockowner.l_owner != current->files)
+ if (pos->lockowner != current->files)
continue;
atomic_inc(&pos->count);
return pos;
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index ed39ee164f5f..da2e6b791316 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -953,7 +953,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
if (test_bit(LK_STATE_IN_USE, &state->flags) == 0)
goto out;
- fl_owner = l_ctx->lockowner.l_owner;
+ fl_owner = l_ctx->lockowner;
fl_flock_owner = l_ctx->open_context->flock_owner;
spin_lock(&state->state_lock);
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 161f8b13bbaa..6e629b856a00 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -867,7 +867,7 @@ static void nfs_pageio_cleanup_mirroring(struct nfs_pageio_descriptor *pgio)
static bool nfs_match_lock_context(const struct nfs_lock_context *l1,
const struct nfs_lock_context *l2)
{
- return l1->lockowner.l_owner == l2->lockowner.l_owner;
+ return l1->lockowner == l2->lockowner;
}
/**
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 4d5897e6d6cb..6e761f3f4cbf 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1151,7 +1151,7 @@ int nfs_flush_incompatible(struct file *file, struct page *page)
if (l_ctx && flctx &&
!(list_empty_careful(&flctx->flc_posix) &&
list_empty_careful(&flctx->flc_flock))) {
- do_flush |= l_ctx->lockowner.l_owner != current->files;
+ do_flush |= l_ctx->lockowner != current->files;
}
nfs_release_request(req);
if (!do_flush)
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 0adb02c4744d..db1002abc95e 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -55,15 +55,11 @@ struct nfs_access_entry {
struct rcu_head rcu_head;
};
-struct nfs_lockowner {
- fl_owner_t l_owner;
-};
-
struct nfs_lock_context {
atomic_t count;
struct list_head list;
struct nfs_open_context *open_context;
- struct nfs_lockowner lockowner;
+ fl_owner_t lockowner;
atomic_t io_count;
};
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.
2016-10-13 4:26 [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2 NeilBrown
2016-10-13 4:26 ` [PATCH 6/6] NFS: discard nfs_lockowner structure NeilBrown
@ 2016-10-13 4:26 ` NeilBrown
2016-11-02 15:49 ` Benjamin Coddington
2016-10-13 4:26 ` [PATCH 2/6] NFSv4: add flock_owner to open context NeilBrown
` (4 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: NeilBrown @ 2016-10-13 4:26 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, Jeff Layton
Cc: Benjamin Coddington, Linux NFS Mailing List
The open_context can always lead directly to the state, and is always easily
available, so this is a straightforward change.
Doing this makes more information available to _nfs4_do_setattr() for use
in the next patch.
Signed-off-by: NeilBrown <neilb@suse.com>
---
fs/nfs/nfs4proc.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 3d85ab7b56bf..950b25413bb4 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -94,7 +94,7 @@ static int nfs4_proc_getattr(struct nfs_server *, struct nfs_fh *, struct nfs_fa
static int _nfs4_proc_getattr(struct nfs_server *server, struct nfs_fh *fhandle, struct nfs_fattr *fattr, struct nfs4_label *label);
static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
struct nfs_fattr *fattr, struct iattr *sattr,
- struct nfs4_state *state, struct nfs4_label *ilabel,
+ struct nfs_open_context *ctx, struct nfs4_label *ilabel,
struct nfs4_label *olabel);
#ifdef CONFIG_NFS_V4_1
static int nfs41_test_stateid(struct nfs_server *, nfs4_stateid *,
@@ -2807,7 +2807,7 @@ static int _nfs4_do_open(struct inode *dir,
nfs_fattr_init(opendata->o_res.f_attr);
status = nfs4_do_setattr(state->inode, cred,
opendata->o_res.f_attr, sattr,
- state, label, olabel);
+ ctx, label, olabel);
if (status == 0) {
nfs_setattr_update_inode(state->inode, sattr,
opendata->o_res.f_attr);
@@ -2902,7 +2902,7 @@ static int _nfs4_do_setattr(struct inode *inode,
struct nfs_setattrargs *arg,
struct nfs_setattrres *res,
struct rpc_cred *cred,
- struct nfs4_state *state)
+ struct nfs_open_context *ctx)
{
struct nfs_server *server = NFS_SERVER(inode);
struct rpc_message msg = {
@@ -2925,13 +2925,13 @@ static int _nfs4_do_setattr(struct inode *inode,
if (nfs4_copy_delegation_stateid(inode, fmode, &arg->stateid, &delegation_cred)) {
/* Use that stateid */
- } else if (truncate && state != NULL) {
+ } else if (truncate && ctx != NULL) {
struct nfs_lockowner lockowner = {
.l_owner = current->files,
};
- if (!nfs4_valid_open_stateid(state))
+ if (!nfs4_valid_open_stateid(ctx->state))
return -EBADF;
- if (nfs4_select_rw_stateid(state, FMODE_WRITE, &lockowner,
+ if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, &lockowner,
&arg->stateid, &delegation_cred) == -EIO)
return -EBADF;
} else
@@ -2942,7 +2942,7 @@ static int _nfs4_do_setattr(struct inode *inode,
status = nfs4_call_sync(server->client, server, &msg, &arg->seq_args, &res->seq_res, 1);
put_rpccred(delegation_cred);
- if (status == 0 && state != NULL)
+ if (status == 0 && ctx != NULL)
renew_lease(server, timestamp);
trace_nfs4_setattr(inode, &arg->stateid, status);
return status;
@@ -2950,10 +2950,11 @@ static int _nfs4_do_setattr(struct inode *inode,
static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
struct nfs_fattr *fattr, struct iattr *sattr,
- struct nfs4_state *state, struct nfs4_label *ilabel,
+ struct nfs_open_context *ctx, struct nfs4_label *ilabel,
struct nfs4_label *olabel)
{
struct nfs_server *server = NFS_SERVER(inode);
+ struct nfs4_state *state = ctx ? ctx->state : NULL;
struct nfs_setattrargs arg = {
.fh = NFS_FH(inode),
.iap = sattr,
@@ -2978,7 +2979,7 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
arg.bitmask = nfs4_bitmask(server, olabel);
do {
- err = _nfs4_do_setattr(inode, &arg, &res, cred, state);
+ err = _nfs4_do_setattr(inode, &arg, &res, cred, ctx);
switch (err) {
case -NFS4ERR_OPENMODE:
if (!(sattr->ia_valid & ATTR_SIZE)) {
@@ -3673,7 +3674,7 @@ nfs4_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
{
struct inode *inode = d_inode(dentry);
struct rpc_cred *cred = NULL;
- struct nfs4_state *state = NULL;
+ struct nfs_open_context *ctx = NULL;
struct nfs4_label *label = NULL;
int status;
@@ -3694,20 +3695,17 @@ nfs4_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
/* Search for an existing open(O_WRITE) file */
if (sattr->ia_valid & ATTR_FILE) {
- struct nfs_open_context *ctx;
ctx = nfs_file_open_context(sattr->ia_file);
- if (ctx) {
+ if (ctx)
cred = ctx->cred;
- state = ctx->state;
- }
}
label = nfs4_label_alloc(NFS_SERVER(inode), GFP_KERNEL);
if (IS_ERR(label))
return PTR_ERR(label);
- status = nfs4_do_setattr(inode, cred, fattr, sattr, state, NULL, label);
+ status = nfs4_do_setattr(inode, cred, fattr, sattr, ctx, NULL, label);
if (status == 0) {
nfs_setattr_update_inode(inode, sattr, fattr);
nfs_setsecurity(inode, fattr, label);
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.
2016-10-13 4:26 ` [PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state NeilBrown
@ 2016-11-02 15:49 ` Benjamin Coddington
2016-11-02 23:34 ` NeilBrown
0 siblings, 1 reply; 19+ messages in thread
From: Benjamin Coddington @ 2016-11-02 15:49 UTC (permalink / raw)
To: NeilBrown
Cc: Trond Myklebust, Anna Schumaker, Jeff Layton, Linux NFS Mailing List
On 13 Oct 2016, at 0:26, NeilBrown wrote:
> The open_context can always lead directly to the state, and is always
> easily
> available, so this is a straightforward change.
> Doing this makes more information available to _nfs4_do_setattr() for
> use
> in the next patch.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> fs/nfs/nfs4proc.c | 28 +++++++++++++---------------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 3d85ab7b56bf..950b25413bb4 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -94,7 +94,7 @@ static int nfs4_proc_getattr(struct nfs_server *,
> struct nfs_fh *, struct nfs_fa
> static int _nfs4_proc_getattr(struct nfs_server *server, struct
> nfs_fh *fhandle, struct nfs_fattr *fattr, struct nfs4_label *label);
> static int nfs4_do_setattr(struct inode *inode, struct rpc_cred
> *cred,
> struct nfs_fattr *fattr, struct iattr *sattr,
> - struct nfs4_state *state, struct nfs4_label *ilabel,
> + struct nfs_open_context *ctx, struct nfs4_label *ilabel,
> struct nfs4_label *olabel);
> #ifdef CONFIG_NFS_V4_1
> static int nfs41_test_stateid(struct nfs_server *, nfs4_stateid *,
> @@ -2807,7 +2807,7 @@ static int _nfs4_do_open(struct inode *dir,
> nfs_fattr_init(opendata->o_res.f_attr);
> status = nfs4_do_setattr(state->inode, cred,
> opendata->o_res.f_attr, sattr,
> - state, label, olabel);
> + ctx, label, olabel);
> if (status == 0) {
> nfs_setattr_update_inode(state->inode, sattr,
> opendata->o_res.f_attr);
> @@ -2902,7 +2902,7 @@ static int _nfs4_do_setattr(struct inode *inode,
> struct nfs_setattrargs *arg,
> struct nfs_setattrres *res,
> struct rpc_cred *cred,
> - struct nfs4_state *state)
> + struct nfs_open_context *ctx)
> {
> struct nfs_server *server = NFS_SERVER(inode);
> struct rpc_message msg = {
> @@ -2925,13 +2925,13 @@ static int _nfs4_do_setattr(struct inode
> *inode,
>
> if (nfs4_copy_delegation_stateid(inode, fmode, &arg->stateid,
> &delegation_cred)) {
> /* Use that stateid */
> - } else if (truncate && state != NULL) {
> + } else if (truncate && ctx != NULL) {
> struct nfs_lockowner lockowner = {
> .l_owner = current->files,
> };
> - if (!nfs4_valid_open_stateid(state))
> + if (!nfs4_valid_open_stateid(ctx->state))
> return -EBADF;
> - if (nfs4_select_rw_stateid(state, FMODE_WRITE, &lockowner,
> + if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, &lockowner,
> &arg->stateid, &delegation_cred) == -EIO)
> return -EBADF;
> } else
> @@ -2942,7 +2942,7 @@ static int _nfs4_do_setattr(struct inode *inode,
> status = nfs4_call_sync(server->client, server, &msg,
> &arg->seq_args, &res->seq_res, 1);
>
> put_rpccred(delegation_cred);
> - if (status == 0 && state != NULL)
> + if (status == 0 && ctx != NULL)
> renew_lease(server, timestamp);
> trace_nfs4_setattr(inode, &arg->stateid, status);
> return status;
> @@ -2950,10 +2950,11 @@ static int _nfs4_do_setattr(struct inode
> *inode,
>
> static int nfs4_do_setattr(struct inode *inode, struct rpc_cred
> *cred,
> struct nfs_fattr *fattr, struct iattr *sattr,
> - struct nfs4_state *state, struct nfs4_label *ilabel,
> + struct nfs_open_context *ctx, struct nfs4_label *ilabel,
> struct nfs4_label *olabel)
> {
> struct nfs_server *server = NFS_SERVER(inode);
> + struct nfs4_state *state = ctx ? ctx->state : NULL;
> struct nfs_setattrargs arg = {
> .fh = NFS_FH(inode),
> .iap = sattr,
> @@ -2978,7 +2979,7 @@ static int nfs4_do_setattr(struct inode *inode,
> struct rpc_cred *cred,
> arg.bitmask = nfs4_bitmask(server, olabel);
>
> do {
> - err = _nfs4_do_setattr(inode, &arg, &res, cred, state);
> + err = _nfs4_do_setattr(inode, &arg, &res, cred, ctx);
> switch (err) {
> case -NFS4ERR_OPENMODE:
> if (!(sattr->ia_valid & ATTR_SIZE)) {
> @@ -3673,7 +3674,7 @@ nfs4_proc_setattr(struct dentry *dentry, struct
> nfs_fattr *fattr,
> {
> struct inode *inode = d_inode(dentry);
> struct rpc_cred *cred = NULL;
> - struct nfs4_state *state = NULL;
> + struct nfs_open_context *ctx = NULL;
> struct nfs4_label *label = NULL;
> int status;
>
> @@ -3694,20 +3695,17 @@ nfs4_proc_setattr(struct dentry *dentry,
> struct nfs_fattr *fattr,
>
> /* Search for an existing open(O_WRITE) file */
> if (sattr->ia_valid & ATTR_FILE) {
> - struct nfs_open_context *ctx;
>
> ctx = nfs_file_open_context(sattr->ia_file);
> - if (ctx) {
> + if (ctx)
> cred = ctx->cred;
> - state = ctx->state;
> - }
> }
Does this need a get_nfs_open_context() there to make sure the open
context
doesn't drop away? I'm getting this on generic/089:
[ 651.855291] run fstests generic/089 at 2016-11-01 11:15:57
[ 652.645828] NFS: nfs4_reclaim_open_state: Lock reclaim failed!
[ 653.166259] NFSD: client ::1 testing state ID with incorrect client
ID
[ 653.167218] BUG: unable to handle kernel NULL pointer dereference at
0000000000000018
[ 653.168142] IP: [<ffffffffa02dbca4>] nfs41_open_expired+0xb4/0x3c0
[nfsv4]
[ 653.168938] PGD 13470c067
[ 653.169236] PUD 132ac1067
[ 653.169557] PMD 0
[ 653.169628]
[ 653.169819] Oops: 0000 [#1] SMP
[ 653.170181] Modules linked in: nfsv4 dns_resolver nfs ip6t_rpfilter
ip6t_REJECT nf_reject_ipv6 xt_conntrack ip_set nfnetlink ebtable_nat
ebtable_broute bridge stp llc ip6table_nat nf_conntrack_ipv6
nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security
ip6table_raw iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4
nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw
ebtable_filter ebtables ip6table_filter ip6_tables nfsd auth_rpcgss
nfs_acl lockd grace sunrpc virtio_net virtio_console virtio_blk
virtio_balloon ppdev crct10dif_pclmul crc32_pclmul crc32c_intel
ghash_clmulni_intel serio_raw virtio_pci parport_pc virtio_ring parport
virtio ata_generic acpi_cpufreq pata_acpi tpm_tis i2c_piix4 tpm_tis_core
tpm
[ 653.178176] CPU: 0 PID: 7508 Comm: ::1-manager Not tainted
4.9.0-rc1-00008-g9b7fe7e #17
[ 653.179068] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.8.1-20150318_183358- 04/01/2014
[ 653.180125] task: ffff880139598000 task.stack: ffffc90008b18000
[ 653.180777] RIP: 0010:[<ffffffffa02dbca4>] [<ffffffffa02dbca4>]
nfs41_open_expired+0xb4/0x3c0 [nfsv4]
[ 653.181829] RSP: 0018:ffffc90008b1bd98 EFLAGS: 00010203
[ 653.182420] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
0000000000000000
[ 653.183212] RDX: 0000000000000035 RSI: ffff88013fc1c9c0 RDI:
ffff880138f2bc00
[ 653.183998] RBP: ffffc90008b1bde8 R08: 000000000001c9c0 R09:
ffff8801319a0300
[ 653.184788] R10: ffff8801319a0338 R11: 0000000000000001 R12:
00000000ffffd8d7
[ 653.185577] R13: ffff88013aaeb6c0 R14: ffff88013aaeb6e0 R15:
ffff88013946f850
[ 653.186458] FS: 0000000000000000(0000) GS:ffff88013fc00000(0000)
knlGS:0000000000000000
[ 653.187329] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 653.187950] CR2: 0000000000000018 CR3: 000000013308c000 CR4:
00000000000406f0
[ 653.188723] Stack:
[ 653.188951] ffff8801319a7800 ffff8801315b9800 ffffc90008b1bda8
ffffc90008b1bda8
[ 653.189807] 000000002f556716 ffff88013aaeb6c0 ffffffffa030b460
ffff88013946f850
[ 653.190666] ffffffffa030b460 ffff88013946f850 ffffc90008b1be80
ffffffffa02f051f
[ 653.191524] Call Trace:
[ 653.191808] [<ffffffffa02f051f>] nfs4_do_reclaim+0x1af/0x660 [nfsv4]
[ 653.192522] [<ffffffffa02f0ee0>] nfs4_run_state_manager+0x510/0x7d0
[nfsv4]
[ 653.193297] [<ffffffffa02f09d0>] ? nfs4_do_reclaim+0x660/0x660
[nfsv4]
[ 653.194016] [<ffffffffa02f09d0>] ? nfs4_do_reclaim+0x660/0x660
[nfsv4]
[ 653.194739] [<ffffffff810c95d9>] kthread+0xd9/0xf0
[ 653.195274] [<ffffffff810c9500>] ? kthread_park+0x60/0x60
[ 653.195875] [<ffffffff818264d5>] ret_from_fork+0x25/0x30
[ 653.196466] Code: 24 49 8b 45 40 a8 01 0f 84 f5 00 00 00 49 8b 5d 20
4d 8d 75 20 49 39 de 75 11 e9 e3 00 00 00 48 8b 1b 4c 39 f3 0f 84 c4 00
00 00 <48> 8b 43 18 a8 01 74 ec 48 8b 43 10 48 8b 3c 24 48 8d b3 08 01
[ 653.199416] RIP [<ffffffffa02dbca4>] nfs41_open_expired+0xb4/0x3c0
[nfsv4]
[ 653.200196] RSP <ffffc90008b1bd98>
[ 653.200578] CR2: 0000000000000018
[ 653.201291] ---[ end trace ad4f1849f777932d ]---
[ 653.201792] Kernel panic - not syncing: Fatal exception
[ 653.202492] Kernel Offset: disabled
[ 653.202876] ---[ end Kernel panic - not syncing: Fatal exception
Something else is also wrong there.. wrapping that with
get_nfs_open_context() makes the crash go away, but there are still
several
"NFS: nfs4_reclaim_open_state: Lock reclaim failed!" in the log. Why
would
we be doing reclaim at all? I'll look at a network capture next.
Ben
>
> label = nfs4_label_alloc(NFS_SERVER(inode), GFP_KERNEL);
> if (IS_ERR(label))
> return PTR_ERR(label);
>
> - status = nfs4_do_setattr(inode, cred, fattr, sattr, state, NULL,
> label);
> + status = nfs4_do_setattr(inode, cred, fattr, sattr, ctx, NULL,
> label);
> if (status == 0) {
> nfs_setattr_update_inode(inode, sattr, fattr);
> nfs_setsecurity(inode, fattr, label);
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.
2016-11-02 15:49 ` Benjamin Coddington
@ 2016-11-02 23:34 ` NeilBrown
2016-11-03 16:38 ` Benjamin Coddington
0 siblings, 1 reply; 19+ messages in thread
From: NeilBrown @ 2016-11-02 23:34 UTC (permalink / raw)
To: Benjamin Coddington
Cc: Trond Myklebust, Anna Schumaker, Jeff Layton, Linux NFS Mailing List
[-- Attachment #1: Type: text/plain, Size: 2307 bytes --]
On Thu, Nov 03 2016, Benjamin Coddington wrote:
> On 13 Oct 2016, at 0:26, NeilBrown wrote:
>> @@ -3694,20 +3695,17 @@ nfs4_proc_setattr(struct dentry *dentry,
>> struct nfs_fattr *fattr,
>>
>> /* Search for an existing open(O_WRITE) file */
>> if (sattr->ia_valid & ATTR_FILE) {
>> - struct nfs_open_context *ctx;
>>
>> ctx = nfs_file_open_context(sattr->ia_file);
>> - if (ctx) {
>> + if (ctx)
>> cred = ctx->cred;
>> - state = ctx->state;
>> - }
>> }
>
>
> Does this need a get_nfs_open_context() there to make sure the open
> context
> doesn't drop away?
I can't see why you would. The ia_file must hold a reference to the
ctx, and this code doesn't keep any reference to the ctx after
nfs4_proc_setattr completes - does it?
> I'm getting this on generic/089:
>
> [ 651.855291] run fstests generic/089 at 2016-11-01 11:15:57
> [ 652.645828] NFS: nfs4_reclaim_open_state: Lock reclaim failed!
> [ 653.166259] NFSD: client ::1 testing state ID with incorrect client
> ID
> [ 653.167218] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000018
I think this BUG is happening in nfs41_check_expired_locks.
This:
list_for_each_entry(lsp, &state->lock_states, ls_locks) {
walks off the end of a list, finding a NULL on a list which should never
have a NULL pointer. That does suggest a use-after-free of an
nfs4_lock_state, or possibly of an nfs4_state.
I can't see it in the code yet though.
>
> Something else is also wrong there.. wrapping that with
> get_nfs_open_context() makes the crash go away, but there are still
> several
> "NFS: nfs4_reclaim_open_state: Lock reclaim failed!" in the log. Why
> would
> we be doing reclaim at all? I'll look at a network capture next.
The
> [ 653.166259] NFSD: client ::1 testing state ID with incorrect client ID
errors suggests that the client is sending a stateid that doesn't match
the client id, so the server reports and error and the client enters
state recovery.
Maybe one thread is dropping a flock lock while another thread is using
it for some IO and they race? I think there are refcounts in place to
protect that but something might be missing.
I look forward to seeing the network capture.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.
2016-11-02 23:34 ` NeilBrown
@ 2016-11-03 16:38 ` Benjamin Coddington
2016-11-03 23:12 ` Benjamin Coddington
0 siblings, 1 reply; 19+ messages in thread
From: Benjamin Coddington @ 2016-11-03 16:38 UTC (permalink / raw)
To: NeilBrown
Cc: Trond Myklebust, Anna Schumaker, Jeff Layton, Linux NFS Mailing List
On 2 Nov 2016, at 19:34, NeilBrown wrote:
> On Thu, Nov 03 2016, Benjamin Coddington wrote:
>
>> On 13 Oct 2016, at 0:26, NeilBrown wrote:
>
>>> @@ -3694,20 +3695,17 @@ nfs4_proc_setattr(struct dentry *dentry,
>>> struct nfs_fattr *fattr,
>>>
>>> /* Search for an existing open(O_WRITE) file */
>>> if (sattr->ia_valid & ATTR_FILE) {
>>> - struct nfs_open_context *ctx;
>>>
>>> ctx = nfs_file_open_context(sattr->ia_file);
>>> - if (ctx) {
>>> + if (ctx)
>>> cred = ctx->cred;
>>> - state = ctx->state;
>>> - }
>>> }
>>
>>
>> Does this need a get_nfs_open_context() there to make sure the open
>> context
>> doesn't drop away?
>
> I can't see why you would. The ia_file must hold a reference to the
> ctx, and this code doesn't keep any reference to the ctx after
> nfs4_proc_setattr completes - does it?
I had a thought that the file pointer could be shared, and just checking to
see if the file's private data was set didn't mean that this task set it.
I'm probably wrong about that, and I'm definitely wrong about the fix, since
it will crash taking a reference. It just took much longer this time.
>> I'm getting this on generic/089:
>>
>> [ 651.855291] run fstests generic/089 at 2016-11-01 11:15:57
>> [ 652.645828] NFS: nfs4_reclaim_open_state: Lock reclaim failed!
>> [ 653.166259] NFSD: client ::1 testing state ID with incorrect client
>> ID
>> [ 653.167218] BUG: unable to handle kernel NULL pointer dereference at
>> 0000000000000018
>
> I think this BUG is happening in nfs41_check_expired_locks.
> This:
> list_for_each_entry(lsp, &state->lock_states, ls_locks) {
> walks off the end of a list, finding a NULL on a list which should never
> have a NULL pointer. That does suggest a use-after-free of an
> nfs4_lock_state, or possibly of an nfs4_state.
>
> I can't see it in the code yet though.
Yes, walking off the list, and I had some idea that we were getting
duplicate fl_owners from a bad lock_context, but today that doesn't make
any sense to me. I can't see how this can happen either.
>>
>> Something else is also wrong there.. wrapping that with
>> get_nfs_open_context() makes the crash go away, but there are still
>> several
>> "NFS: nfs4_reclaim_open_state: Lock reclaim failed!" in the log. Why
>> would
>> we be doing reclaim at all? I'll look at a network capture next.
>
> The
>> [ 653.166259] NFSD: client ::1 testing state ID with incorrect client ID
>
> errors suggests that the client is sending a stateid that doesn't match
> the client id, so the server reports and error and the client enters
> state recovery.
> Maybe one thread is dropping a flock lock while another thread is using
> it for some IO and they race? I think there are refcounts in place to
> protect that but something might be missing.
>
> I look forward to seeing the network capture.
The network capture isn't very enlightening.. or I haven't been able to find
where the problem happens in the 800Mb of capture. I probably need to make
the box panic on oops and capture externally so the problem is right at the
end of the capture. I'll keep plugging away at this as I can.
Ben
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.
2016-11-03 16:38 ` Benjamin Coddington
@ 2016-11-03 23:12 ` Benjamin Coddington
0 siblings, 0 replies; 19+ messages in thread
From: Benjamin Coddington @ 2016-11-03 23:12 UTC (permalink / raw)
To: NeilBrown
Cc: Trond Myklebust, Anna Schumaker, Jeff Layton, Linux NFS Mailing List
On 3 Nov 2016, at 12:38, Benjamin Coddington wrote:
> The network capture isn't very enlightening.. or I haven't been able
> to find
> where the problem happens in the 800Mb of capture. I probably need to
> make
> the box panic on oops and capture externally so the problem is right
> at the
> end of the capture. I'll keep plugging away at this as I can.
I just got this on v4.9-rc1 without these patches, so false alarm here.
Sorry
for the noise. I'll see if I can bisect.
Ben
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/6] NFSv4: add flock_owner to open context
2016-10-13 4:26 [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2 NeilBrown
2016-10-13 4:26 ` [PATCH 6/6] NFS: discard nfs_lockowner structure NeilBrown
2016-10-13 4:26 ` [PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state NeilBrown
@ 2016-10-13 4:26 ` NeilBrown
2016-10-13 4:26 ` [PATCH 4/6] NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner NeilBrown
` (3 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: NeilBrown @ 2016-10-13 4:26 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, Jeff Layton
Cc: Benjamin Coddington, Linux NFS Mailing List
An open file description (struct file) in a given process can be
associated with two different lock owners.
It can have a Posix lock owner which will be different in each process
that has a fd on the file.
It can have a Flock owner which will be the same in all processes.
When searching for a lock stateid to use, we need to consider both of these
owners
So add a new "flock_owner" to the "nfs_open_context" (of which there
is one for each open file description).
This flock_owner does not need to be reference-counted as there is a
1-1 relation between 'struct file' and nfs open contexts,
and it will never be part of a list of contexts. So there is no need
for a 'flock_context' - just the owner is enough.
The io_count included in the (Posix) lock_context provides no
guarantee that all read-aheads that could use the state have
completed, so not supporting it for flock locks in not a serious
problem. Synchronization between flock and read-ahead can be added
later if needed.
When creating an open_context for a non-openning create call, we don't have
a 'struct file' to pass in, so the lock context gets initialized with
a NULL owner, but this will never be used.
The flock_owner is not used at all in this patch, that will come later.
Acked-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
fs/nfs/dir.c | 6 +++---
fs/nfs/inode.c | 7 +++++--
fs/nfs/nfs4file.c | 2 +-
fs/nfs/nfs4proc.c | 2 +-
include/linux/nfs_fs.h | 3 ++-
5 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 5f1af4cd1a33..f1ecfcc632eb 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1467,9 +1467,9 @@ static fmode_t flags_to_mode(int flags)
return res;
}
-static struct nfs_open_context *create_nfs_open_context(struct dentry *dentry, int open_flags)
+static struct nfs_open_context *create_nfs_open_context(struct dentry *dentry, int open_flags, struct file *filp)
{
- return alloc_nfs_open_context(dentry, flags_to_mode(open_flags));
+ return alloc_nfs_open_context(dentry, flags_to_mode(open_flags), filp);
}
static int do_open(struct inode *inode, struct file *filp)
@@ -1554,7 +1554,7 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
return finish_no_open(file, dentry);
}
- ctx = create_nfs_open_context(dentry, open_flags);
+ ctx = create_nfs_open_context(dentry, open_flags, file);
err = PTR_ERR(ctx);
if (IS_ERR(ctx))
goto out;
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 1752fc7c0024..2138027eaba9 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -796,7 +796,9 @@ void nfs_close_context(struct nfs_open_context *ctx, int is_sync)
}
EXPORT_SYMBOL_GPL(nfs_close_context);
-struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode)
+struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry,
+ fmode_t f_mode,
+ struct file *filp)
{
struct nfs_open_context *ctx;
struct rpc_cred *cred = rpc_lookup_cred();
@@ -815,6 +817,7 @@ struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f
ctx->mode = f_mode;
ctx->flags = 0;
ctx->error = 0;
+ ctx->flock_owner = (fl_owner_t)filp;
nfs_init_lock_context(&ctx->lock_context);
ctx->lock_context.open_context = ctx;
INIT_LIST_HEAD(&ctx->list);
@@ -939,7 +942,7 @@ int nfs_open(struct inode *inode, struct file *filp)
{
struct nfs_open_context *ctx;
- ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
+ ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode, filp);
if (IS_ERR(ctx))
return PTR_ERR(ctx);
nfs_file_set_open_context(filp, ctx);
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 89a77950e0b0..0efba77789b9 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, struct file *filp)
parent = dget_parent(dentry);
dir = d_inode(parent);
- ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
+ ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode, filp);
err = PTR_ERR(ctx);
if (IS_ERR(ctx))
goto out;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 3c164f9a3983..3d85ab7b56bf 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3957,7 +3957,7 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
struct nfs4_state *state;
int status = 0;
- ctx = alloc_nfs_open_context(dentry, FMODE_READ);
+ ctx = alloc_nfs_open_context(dentry, FMODE_READ, NULL);
if (IS_ERR(ctx))
return PTR_ERR(ctx);
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index bf8a713c45b4..0adb02c4744d 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -70,6 +70,7 @@ struct nfs_lock_context {
struct nfs4_state;
struct nfs_open_context {
struct nfs_lock_context lock_context;
+ fl_owner_t flock_owner;
struct dentry *dentry;
struct rpc_cred *cred;
struct nfs4_state *state;
@@ -357,7 +358,7 @@ extern void nfs_setsecurity(struct inode *inode, struct nfs_fattr *fattr,
extern struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx);
extern void put_nfs_open_context(struct nfs_open_context *ctx);
extern struct nfs_open_context *nfs_find_open_context(struct inode *inode, struct rpc_cred *cred, fmode_t mode);
-extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode);
+extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode, struct file *filp);
extern void nfs_inode_attach_open_context(struct nfs_open_context *ctx);
extern void nfs_file_set_open_context(struct file *filp, struct nfs_open_context *ctx);
extern void nfs_file_clear_open_context(struct file *flip);
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 4/6] NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner
2016-10-13 4:26 [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2 NeilBrown
` (2 preceding siblings ...)
2016-10-13 4:26 ` [PATCH 2/6] NFSv4: add flock_owner to open context NeilBrown
@ 2016-10-13 4:26 ` NeilBrown
2016-10-20 0:57 ` NeilBrown
2016-10-13 4:26 ` [PATCH 1/6] NFS: remove l_pid field from nfs_lockowner NeilBrown
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: NeilBrown @ 2016-10-13 4:26 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, Jeff Layton
Cc: Benjamin Coddington, Linux NFS Mailing List
The only time that a lock_context is not immediately available is in
setattr, and now that it has an open_context, it can easily find one
with nfs_get_lock_context.
This removes the need for the on-stack nfs_lockowner.
This change is preparation for correctly support flock stateids.
Signed-off-by: NeilBrown <neilb@suse.com>
---
fs/nfs/nfs4_fs.h | 2 +-
fs/nfs/nfs4proc.c | 15 ++++++---------
fs/nfs/nfs4state.c | 11 +++++------
3 files changed, 12 insertions(+), 16 deletions(-)
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 9b3a82abab07..7784b79915e1 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -457,7 +457,7 @@ extern void nfs41_handle_server_scope(struct nfs_client *,
extern void nfs4_put_lock_state(struct nfs4_lock_state *lsp);
extern int nfs4_set_lock_state(struct nfs4_state *state, struct file_lock *fl);
extern int nfs4_select_rw_stateid(struct nfs4_state *, fmode_t,
- const struct nfs_lockowner *, nfs4_stateid *,
+ const struct nfs_lock_context *, nfs4_stateid *,
struct rpc_cred **);
extern struct nfs_seqid *nfs_alloc_seqid(struct nfs_seqid_counter *counter, gfp_t gfp_mask);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 950b25413bb4..3c2f11189794 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2926,12 +2926,13 @@ static int _nfs4_do_setattr(struct inode *inode,
if (nfs4_copy_delegation_stateid(inode, fmode, &arg->stateid, &delegation_cred)) {
/* Use that stateid */
} else if (truncate && ctx != NULL) {
- struct nfs_lockowner lockowner = {
- .l_owner = current->files,
- };
+ struct nfs_lock_context *l_ctx;
if (!nfs4_valid_open_stateid(ctx->state))
return -EBADF;
- if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, &lockowner,
+ l_ctx = nfs_get_lock_context(ctx);
+ if (IS_ERR(l_ctx))
+ return PTR_ERR(l_ctx);
+ if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, l_ctx,
&arg->stateid, &delegation_cred) == -EIO)
return -EBADF;
} else
@@ -4525,11 +4526,7 @@ int nfs4_set_rw_stateid(nfs4_stateid *stateid,
const struct nfs_lock_context *l_ctx,
fmode_t fmode)
{
- const struct nfs_lockowner *lockowner = NULL;
-
- if (l_ctx != NULL)
- lockowner = &l_ctx->lockowner;
- return nfs4_select_rw_stateid(ctx->state, fmode, lockowner, stateid, NULL);
+ return nfs4_select_rw_stateid(ctx->state, fmode, l_ctx, stateid, NULL);
}
EXPORT_SYMBOL_GPL(nfs4_set_rw_stateid);
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 5f4281ec5f72..f25eee8202bf 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -939,20 +939,19 @@ int nfs4_set_lock_state(struct nfs4_state *state, struct file_lock *fl)
static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
struct nfs4_state *state,
- const struct nfs_lockowner *lockowner)
+ const struct nfs_lock_context *l_ctx)
{
struct nfs4_lock_state *lsp;
fl_owner_t fl_owner;
int ret = -ENOENT;
-
- if (lockowner == NULL)
+ if (l_ctx == NULL)
goto out;
if (test_bit(LK_STATE_IN_USE, &state->flags) == 0)
goto out;
- fl_owner = lockowner->l_owner;
+ fl_owner = l_ctx->lockowner.l_owner;
spin_lock(&state->state_lock);
lsp = __nfs4_find_lock_state(state, fl_owner);
if (lsp && test_bit(NFS_LOCK_LOST, &lsp->ls_flags))
@@ -986,7 +985,7 @@ static void nfs4_copy_open_stateid(nfs4_stateid *dst, struct nfs4_state *state)
* requests.
*/
int nfs4_select_rw_stateid(struct nfs4_state *state,
- fmode_t fmode, const struct nfs_lockowner *lockowner,
+ fmode_t fmode, const struct nfs_lock_context *l_ctx,
nfs4_stateid *dst, struct rpc_cred **cred)
{
int ret;
@@ -995,7 +994,7 @@ int nfs4_select_rw_stateid(struct nfs4_state *state,
return -EIO;
if (cred != NULL)
*cred = NULL;
- ret = nfs4_copy_lock_stateid(dst, state, lockowner);
+ ret = nfs4_copy_lock_stateid(dst, state, l_ctx);
if (ret == -EIO)
/* A lost lock - don't even consider delegations */
goto out;
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/6] NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner
2016-10-13 4:26 ` [PATCH 4/6] NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner NeilBrown
@ 2016-10-20 0:57 ` NeilBrown
0 siblings, 0 replies; 19+ messages in thread
From: NeilBrown @ 2016-10-20 0:57 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, Jeff Layton
Cc: Benjamin Coddington, Linux NFS Mailing List
[-- Attachment #1: Type: text/plain, Size: 3138 bytes --]
On Thu, Oct 13 2016, NeilBrown wrote:
> The only time that a lock_context is not immediately available is in
> setattr, and now that it has an open_context, it can easily find one
> with nfs_get_lock_context.
> This removes the need for the on-stack nfs_lockowner.
>
> This change is preparation for correctly support flock stateids.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> fs/nfs/nfs4_fs.h | 2 +-
> fs/nfs/nfs4proc.c | 15 ++++++---------
> fs/nfs/nfs4state.c | 11 +++++------
> 3 files changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index 9b3a82abab07..7784b79915e1 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -457,7 +457,7 @@ extern void nfs41_handle_server_scope(struct nfs_client *,
> extern void nfs4_put_lock_state(struct nfs4_lock_state *lsp);
> extern int nfs4_set_lock_state(struct nfs4_state *state, struct file_lock *fl);
> extern int nfs4_select_rw_stateid(struct nfs4_state *, fmode_t,
> - const struct nfs_lockowner *, nfs4_stateid *,
> + const struct nfs_lock_context *, nfs4_stateid *,
> struct rpc_cred **);
>
> extern struct nfs_seqid *nfs_alloc_seqid(struct nfs_seqid_counter *counter, gfp_t gfp_mask);
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 950b25413bb4..3c2f11189794 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2926,12 +2926,13 @@ static int _nfs4_do_setattr(struct inode *inode,
> if (nfs4_copy_delegation_stateid(inode, fmode, &arg->stateid, &delegation_cred)) {
> /* Use that stateid */
> } else if (truncate && ctx != NULL) {
> - struct nfs_lockowner lockowner = {
> - .l_owner = current->files,
> - };
> + struct nfs_lock_context *l_ctx;
> if (!nfs4_valid_open_stateid(ctx->state))
> return -EBADF;
> - if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, &lockowner,
> + l_ctx = nfs_get_lock_context(ctx);
> + if (IS_ERR(l_ctx))
> + return PTR_ERR(l_ctx);
> + if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, l_ctx,
> &arg->stateid, &delegation_cred) == -EIO)
> return -EBADF;
Oops...
I have an nfs_get_lock_context() here with no matching
nfs_put_lock_context();
I've merged:
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 3c2f11189794..be4e3d9de1eb 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2932,8 +2932,10 @@ static int _nfs4_do_setattr(struct inode *inode,
l_ctx = nfs_get_lock_context(ctx);
if (IS_ERR(l_ctx))
return PTR_ERR(l_ctx);
- if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, l_ctx,
- &arg->stateid, &delegation_cred) == -EIO)
+ status = nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, l_ctx,
+ &arg->stateid, &delegation_cred);
+ nfs_put_lock_context(l_ctx);
+ if (status == -EIO)
return -EBADF;
} else
nfs4_stateid_copy(&arg->stateid, &zero_stateid);
into that patch and pushed out a new
git://neil.brown.name/linux tags/nfs-flock-fix
(3f5546538d7c678b82d10ce6add29a6d92565f1e)
Please let me know if I should re-post the series.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 1/6] NFS: remove l_pid field from nfs_lockowner
2016-10-13 4:26 [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2 NeilBrown
` (3 preceding siblings ...)
2016-10-13 4:26 ` [PATCH 4/6] NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner NeilBrown
@ 2016-10-13 4:26 ` NeilBrown
2016-10-13 4:26 ` [PATCH 5/6] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one NeilBrown
2016-10-13 15:31 ` [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2 Jeff Layton
6 siblings, 0 replies; 19+ messages in thread
From: NeilBrown @ 2016-10-13 4:26 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, Jeff Layton
Cc: Benjamin Coddington, Linux NFS Mailing List
this field is not used in any important way and probably should
have been removed by
Commit: 8003d3c4aaa5 ("nfs4: treat lock owners as opaque values")
which removed the pid argument from nfs4_get_lock_state.
Except in unusual and uninteresting cases, two threads with the same
->tgid will have the same ->files pointer, so keeping them both
for comparison brings no benefit.
Acked-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
fs/nfs/inode.c | 3 ---
fs/nfs/nfs4proc.c | 1 -
fs/nfs/pagelist.c | 3 +--
fs/nfs/write.c | 3 +--
include/linux/nfs_fs.h | 1 -
5 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index bf4ec5ecc97e..1752fc7c0024 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -703,7 +703,6 @@ static void nfs_init_lock_context(struct nfs_lock_context *l_ctx)
{
atomic_set(&l_ctx->count, 1);
l_ctx->lockowner.l_owner = current->files;
- l_ctx->lockowner.l_pid = current->tgid;
INIT_LIST_HEAD(&l_ctx->list);
atomic_set(&l_ctx->io_count, 0);
}
@@ -716,8 +715,6 @@ static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context
do {
if (pos->lockowner.l_owner != current->files)
continue;
- if (pos->lockowner.l_pid != current->tgid)
- continue;
atomic_inc(&pos->count);
return pos;
} while ((pos = list_entry(pos->list.next, typeof(*pos), list)) != head);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ad917bd72b38..3c164f9a3983 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2928,7 +2928,6 @@ static int _nfs4_do_setattr(struct inode *inode,
} else if (truncate && state != NULL) {
struct nfs_lockowner lockowner = {
.l_owner = current->files,
- .l_pid = current->tgid,
};
if (!nfs4_valid_open_stateid(state))
return -EBADF;
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 965db474f4b0..161f8b13bbaa 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -867,8 +867,7 @@ static void nfs_pageio_cleanup_mirroring(struct nfs_pageio_descriptor *pgio)
static bool nfs_match_lock_context(const struct nfs_lock_context *l1,
const struct nfs_lock_context *l2)
{
- return l1->lockowner.l_owner == l2->lockowner.l_owner
- && l1->lockowner.l_pid == l2->lockowner.l_pid;
+ return l1->lockowner.l_owner == l2->lockowner.l_owner;
}
/**
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 53211838f72a..4d5897e6d6cb 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1151,8 +1151,7 @@ int nfs_flush_incompatible(struct file *file, struct page *page)
if (l_ctx && flctx &&
!(list_empty_careful(&flctx->flc_posix) &&
list_empty_careful(&flctx->flc_flock))) {
- do_flush |= l_ctx->lockowner.l_owner != current->files
- || l_ctx->lockowner.l_pid != current->tgid;
+ do_flush |= l_ctx->lockowner.l_owner != current->files;
}
nfs_release_request(req);
if (!do_flush)
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 810124b33327..bf8a713c45b4 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -57,7 +57,6 @@ struct nfs_access_entry {
struct nfs_lockowner {
fl_owner_t l_owner;
- pid_t l_pid;
};
struct nfs_lock_context {
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 5/6] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one
2016-10-13 4:26 [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2 NeilBrown
` (4 preceding siblings ...)
2016-10-13 4:26 ` [PATCH 1/6] NFS: remove l_pid field from nfs_lockowner NeilBrown
@ 2016-10-13 4:26 ` NeilBrown
2016-10-13 15:22 ` Jeff Layton
2016-10-13 15:31 ` [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2 Jeff Layton
6 siblings, 1 reply; 19+ messages in thread
From: NeilBrown @ 2016-10-13 4:26 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, Jeff Layton
Cc: Benjamin Coddington, Linux NFS Mailing List
A process can have two possible lock owner for a given open file:
a per-process Posix lock owner and a per-open-file flock owner
Use both of these when searching for a suitable stateid to use.
With this patch, READ/WRITE requests will use the correct stateid
if a flock lock is active.
Signed-off-by: NeilBrown <neilb@suse.com>
---
fs/nfs/nfs4state.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index f25eee8202bf..ed39ee164f5f 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -800,11 +800,13 @@ void nfs4_close_sync(struct nfs4_state *state, fmode_t fmode)
* that is compatible with current->files
*/
static struct nfs4_lock_state *
-__nfs4_find_lock_state(struct nfs4_state *state, fl_owner_t fl_owner)
+__nfs4_find_lock_state(struct nfs4_state *state,
+ fl_owner_t fl_owner, fl_owner_t fl_owner2)
{
struct nfs4_lock_state *pos;
list_for_each_entry(pos, &state->lock_states, ls_locks) {
- if (pos->ls_owner != fl_owner)
+ if (pos->ls_owner != fl_owner &&
+ pos->ls_owner != fl_owner2)
continue;
atomic_inc(&pos->ls_count);
return pos;
@@ -857,7 +859,7 @@ static struct nfs4_lock_state *nfs4_get_lock_state(struct nfs4_state *state, fl_
for(;;) {
spin_lock(&state->state_lock);
- lsp = __nfs4_find_lock_state(state, owner);
+ lsp = __nfs4_find_lock_state(state, owner, 0);
if (lsp != NULL)
break;
if (new != NULL) {
@@ -942,7 +944,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
const struct nfs_lock_context *l_ctx)
{
struct nfs4_lock_state *lsp;
- fl_owner_t fl_owner;
+ fl_owner_t fl_owner, fl_flock_owner;
int ret = -ENOENT;
if (l_ctx == NULL)
@@ -952,8 +954,10 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
goto out;
fl_owner = l_ctx->lockowner.l_owner;
+ fl_flock_owner = l_ctx->open_context->flock_owner;
+
spin_lock(&state->state_lock);
- lsp = __nfs4_find_lock_state(state, fl_owner);
+ lsp = __nfs4_find_lock_state(state, fl_owner, fl_flock_owner);
if (lsp && test_bit(NFS_LOCK_LOST, &lsp->ls_flags))
ret = -EIO;
else if (lsp != NULL && test_bit(NFS_LOCK_INITIALIZED, &lsp->ls_flags) != 0) {
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 5/6] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one
2016-10-13 4:26 ` [PATCH 5/6] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one NeilBrown
@ 2016-10-13 15:22 ` Jeff Layton
2016-10-14 0:22 ` NeilBrown
0 siblings, 1 reply; 19+ messages in thread
From: Jeff Layton @ 2016-10-13 15:22 UTC (permalink / raw)
To: NeilBrown, Trond Myklebust, Anna Schumaker
Cc: Benjamin Coddington, Linux NFS Mailing List
On Thu, 2016-10-13 at 15:26 +1100, NeilBrown wrote:
> A process can have two possible lock owner for a given open file:
> a per-process Posix lock owner and a per-open-file flock owner
> Use both of these when searching for a suitable stateid to use.
>
> With this patch, READ/WRITE requests will use the correct stateid
> if a flock lock is active.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> fs/nfs/nfs4state.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index f25eee8202bf..ed39ee164f5f 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -800,11 +800,13 @@ void nfs4_close_sync(struct nfs4_state *state, fmode_t fmode)
> * that is compatible with current->files
> */
> static struct nfs4_lock_state *
> -__nfs4_find_lock_state(struct nfs4_state *state, fl_owner_t fl_owner)
> +__nfs4_find_lock_state(struct nfs4_state *state,
> + fl_owner_t fl_owner, fl_owner_t fl_owner2)
> {
> struct nfs4_lock_state *pos;
> list_for_each_entry(pos, &state->lock_states, ls_locks) {
> - if (pos->ls_owner != fl_owner)
> + if (pos->ls_owner != fl_owner &&
> + pos->ls_owner != fl_owner2)
> continue;
> atomic_inc(&pos->ls_count);
> return pos;
Ok, so we end up getting whatever is first on the list here. That's
certainly fine when there are either flock/OFD locks or traditional
POSIX locks in use.
When there are both in use though, then things may be less predictable.
That said, mixing flock/OFD and POSIX locks on the same fds from the
same process is not a great idea in general, and I have a hard time
coming up with a valid use-case there.
So, I don't see that as a real problem, but it may be worth explaining
that rationale in the comment block above this function in case we need
to revisit it later.
> @@ -857,7 +859,7 @@ static struct nfs4_lock_state *nfs4_get_lock_state(struct nfs4_state *state, fl_
>
> for(;;) {
> spin_lock(&state->state_lock);
> - lsp = __nfs4_find_lock_state(state, owner);
> + lsp = __nfs4_find_lock_state(state, owner, 0);
> if (lsp != NULL)
> break;
> if (new != NULL) {
> @@ -942,7 +944,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
> const struct nfs_lock_context *l_ctx)
> {
> struct nfs4_lock_state *lsp;
> - fl_owner_t fl_owner;
> + fl_owner_t fl_owner, fl_flock_owner;
> int ret = -ENOENT;
>
> if (l_ctx == NULL)
> @@ -952,8 +954,10 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
> goto out;
>
> fl_owner = l_ctx->lockowner.l_owner;
> + fl_flock_owner = l_ctx->open_context->flock_owner;
> +
> spin_lock(&state->state_lock);
> - lsp = __nfs4_find_lock_state(state, fl_owner);
> + lsp = __nfs4_find_lock_state(state, fl_owner, fl_flock_owner);
> if (lsp && test_bit(NFS_LOCK_LOST, &lsp->ls_flags))
> ret = -EIO;
> else if (lsp != NULL && test_bit(NFS_LOCK_INITIALIZED, &lsp->ls_flags) != 0) {
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 5/6] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one
2016-10-13 15:22 ` Jeff Layton
@ 2016-10-14 0:22 ` NeilBrown
2016-10-14 10:49 ` Jeff Layton
0 siblings, 1 reply; 19+ messages in thread
From: NeilBrown @ 2016-10-14 0:22 UTC (permalink / raw)
To: Jeff Layton, Trond Myklebust, Anna Schumaker
Cc: Benjamin Coddington, Linux NFS Mailing List
[-- Attachment #1: Type: text/plain, Size: 5894 bytes --]
On Fri, Oct 14 2016, Jeff Layton wrote:
> On Thu, 2016-10-13 at 15:26 +1100, NeilBrown wrote:
>> A process can have two possible lock owner for a given open file:
>> a per-process Posix lock owner and a per-open-file flock owner
>> Use both of these when searching for a suitable stateid to use.
>>
>> With this patch, READ/WRITE requests will use the correct stateid
>> if a flock lock is active.
>>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>> fs/nfs/nfs4state.c | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index f25eee8202bf..ed39ee164f5f 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -800,11 +800,13 @@ void nfs4_close_sync(struct nfs4_state *state, fmode_t fmode)
>> * that is compatible with current->files
>> */
>> static struct nfs4_lock_state *
>> -__nfs4_find_lock_state(struct nfs4_state *state, fl_owner_t fl_owner)
>> +__nfs4_find_lock_state(struct nfs4_state *state,
>> + fl_owner_t fl_owner, fl_owner_t fl_owner2)
>> {
>> struct nfs4_lock_state *pos;
>> list_for_each_entry(pos, &state->lock_states, ls_locks) {
>> - if (pos->ls_owner != fl_owner)
>> + if (pos->ls_owner != fl_owner &&
>> + pos->ls_owner != fl_owner2)
>> continue;
>> atomic_inc(&pos->ls_count);
>> return pos;
>
> Ok, so we end up getting whatever is first on the list here. That's
> certainly fine when there are either flock/OFD locks or traditional
> POSIX locks in use.
>
> When there are both in use though, then things may be less predictable.
> That said, mixing flock/OFD and POSIX locks on the same fds from the
> same process is not a great idea in general, and I have a hard time
> coming up with a valid use-case there.
Using two types of locks in the one application would be ... unusual.
I wouldn't want to spend much of addressing any issues, but not being
predictable isn't good. Intermittent problems are so hard to debug.
We should probably make sure it consistently chooses on or the other.
As flock locks are always whole-file, it is always safe to choose the
flock lock over the posix lock as you can be sure the IO is covered by
the lock. OFD locks make that a little be less of a clear choice.
On the other hand, NFS locks were originally only Posix locks and flock
locks were only supported much later. So for historical consistency we
should probably choose the Posix stateid preferentially.
I find the second argument more convincing. Here is the updated patch.
Thanks,
NeilBrown
From: NeilBrown <neilb@suse.com>
Subject: [PATCH] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid
if there is one
A process can have two possible lock owner for a given open file:
a per-process Posix lock owner and a per-open-file flock owner
Use both of these when searching for a suitable stateid to use.
With this patch, READ/WRITE requests will use the correct stateid
if a flock lock is active.
Signed-off-by: NeilBrown <neilb@suse.com>
---
fs/nfs/nfs4state.c | 38 +++++++++++++++++++++++++++-----------
1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index f25eee8202bf..bd29d4360660 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -797,19 +797,33 @@ void nfs4_close_sync(struct nfs4_state *state, fmode_t fmode)
/*
* Search the state->lock_states for an existing lock_owner
- * that is compatible with current->files
+ * that is compatible with either of the given owners.
+ * If the second is non-zero, then the first refers to a Posix-lock
+ * owner (current->files) and the second refers to a flock/OFD
+ * owner (struct file*). In that case, prefer a match for the first
+ * owner.
+ * If both sorts of locks are held on the one file we cannot know
+ * which stateid was intended to be used, so a "correct" choice cannot
+ * be made. Failing that, a "consistent" choice is preferable. The
+ * consistent choice we make is to prefer the first owner, that of a
+ * Posix lock.
*/
static struct nfs4_lock_state *
-__nfs4_find_lock_state(struct nfs4_state *state, fl_owner_t fl_owner)
+__nfs4_find_lock_state(struct nfs4_state *state,
+ fl_owner_t fl_owner, fl_owner_t fl_owner2)
{
- struct nfs4_lock_state *pos;
+ struct nfs4_lock_state *pos, *ret = NULL;
list_for_each_entry(pos, &state->lock_states, ls_locks) {
- if (pos->ls_owner != fl_owner)
- continue;
- atomic_inc(&pos->ls_count);
- return pos;
+ if (pos->ls_owner == fl_owner) {
+ ret = pos;
+ break;
+ }
+ if (pos->ls_owner == fl_owner2)
+ ret = pos;
}
- return NULL;
+ if (ret)
+ atomic_inc(&ret->ls_count);
+ return ret;
}
/*
@@ -857,7 +871,7 @@ static struct nfs4_lock_state *nfs4_get_lock_state(struct nfs4_state *state, fl_
for(;;) {
spin_lock(&state->state_lock);
- lsp = __nfs4_find_lock_state(state, owner);
+ lsp = __nfs4_find_lock_state(state, owner, 0);
if (lsp != NULL)
break;
if (new != NULL) {
@@ -942,7 +956,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
const struct nfs_lock_context *l_ctx)
{
struct nfs4_lock_state *lsp;
- fl_owner_t fl_owner;
+ fl_owner_t fl_owner, fl_flock_owner;
int ret = -ENOENT;
if (l_ctx == NULL)
@@ -952,8 +966,10 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
goto out;
fl_owner = l_ctx->lockowner.l_owner;
+ fl_flock_owner = l_ctx->open_context->flock_owner;
+
spin_lock(&state->state_lock);
- lsp = __nfs4_find_lock_state(state, fl_owner);
+ lsp = __nfs4_find_lock_state(state, fl_owner, fl_flock_owner);
if (lsp && test_bit(NFS_LOCK_LOST, &lsp->ls_flags))
ret = -EIO;
else if (lsp != NULL && test_bit(NFS_LOCK_INITIALIZED, &lsp->ls_flags) != 0) {
--
2.10.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 5/6] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one
2016-10-14 0:22 ` NeilBrown
@ 2016-10-14 10:49 ` Jeff Layton
2016-12-19 0:33 ` [PATCH] NFSv4: ensure __nfs4_find_lock_state returns consistent result NeilBrown
0 siblings, 1 reply; 19+ messages in thread
From: Jeff Layton @ 2016-10-14 10:49 UTC (permalink / raw)
To: NeilBrown, Trond Myklebust, Anna Schumaker
Cc: Benjamin Coddington, Linux NFS Mailing List
On Fri, 2016-10-14 at 11:22 +1100, NeilBrown wrote:
> On Fri, Oct 14 2016, Jeff Layton wrote:
>
> >
> > On Thu, 2016-10-13 at 15:26 +1100, NeilBrown wrote:
> > >
> > > A process can have two possible lock owner for a given open file:
> > > a per-process Posix lock owner and a per-open-file flock owner
> > > Use both of these when searching for a suitable stateid to use.
> > >
> > > With this patch, READ/WRITE requests will use the correct stateid
> > > if a flock lock is active.
> > >
> > > Signed-off-by: NeilBrown <neilb@suse.com>
> > > ---
> > > fs/nfs/nfs4state.c | 14 +++++++++-----
> > > 1 file changed, 9 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > > index f25eee8202bf..ed39ee164f5f 100644
> > > --- a/fs/nfs/nfs4state.c
> > > +++ b/fs/nfs/nfs4state.c
> > > @@ -800,11 +800,13 @@ void nfs4_close_sync(struct nfs4_state *state, fmode_t fmode)
> > > * that is compatible with current->files
> > > */
> > > static struct nfs4_lock_state *
> > > -__nfs4_find_lock_state(struct nfs4_state *state, fl_owner_t fl_owner)
> > > +__nfs4_find_lock_state(struct nfs4_state *state,
> > > + fl_owner_t fl_owner, fl_owner_t fl_owner2)
> > > {
> > > struct nfs4_lock_state *pos;
> > > list_for_each_entry(pos, &state->lock_states, ls_locks) {
> > > - if (pos->ls_owner != fl_owner)
> > > + if (pos->ls_owner != fl_owner &&
> > > + pos->ls_owner != fl_owner2)
> > > continue;
> > > atomic_inc(&pos->ls_count);
> > > return pos;
> >
> > Ok, so we end up getting whatever is first on the list here. That's
> > certainly fine when there are either flock/OFD locks or traditional
> > POSIX locks in use.
> >
> > When there are both in use though, then things may be less predictable.
> > That said, mixing flock/OFD and POSIX locks on the same fds from the
> > same process is not a great idea in general, and I have a hard time
> > coming up with a valid use-case there.
>
> Using two types of locks in the one application would be ... unusual.
> I wouldn't want to spend much of addressing any issues, but not being
> predictable isn't good. Intermittent problems are so hard to debug.
>
> We should probably make sure it consistently chooses on or the other.
> As flock locks are always whole-file, it is always safe to choose the
> flock lock over the posix lock as you can be sure the IO is covered by
> the lock. OFD locks make that a little be less of a clear choice.
>
> On the other hand, NFS locks were originally only Posix locks and flock
> locks were only supported much later. So for historical consistency we
> should probably choose the Posix stateid preferentially.
>
> I find the second argument more convincing. Here is the updated patch.
>
> Thanks,
> NeilBrown
>
> From: NeilBrown <neilb@suse.com>
> Subject: [PATCH] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid
> if there is one
>
> A process can have two possible lock owner for a given open file:
> a per-process Posix lock owner and a per-open-file flock owner
> Use both of these when searching for a suitable stateid to use.
>
> With this patch, READ/WRITE requests will use the correct stateid
> if a flock lock is active.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> fs/nfs/nfs4state.c | 38 +++++++++++++++++++++++++++-----------
> 1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index f25eee8202bf..bd29d4360660 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -797,19 +797,33 @@ void nfs4_close_sync(struct nfs4_state *state, fmode_t fmode)
>
> /*
> * Search the state->lock_states for an existing lock_owner
> - * that is compatible with current->files
> + * that is compatible with either of the given owners.
> + * If the second is non-zero, then the first refers to a Posix-lock
> + * owner (current->files) and the second refers to a flock/OFD
> + * owner (struct file*). In that case, prefer a match for the first
> + * owner.
> + * If both sorts of locks are held on the one file we cannot know
> + * which stateid was intended to be used, so a "correct" choice cannot
> + * be made. Failing that, a "consistent" choice is preferable. The
> + * consistent choice we make is to prefer the first owner, that of a
> + * Posix lock.
> */
> static struct nfs4_lock_state *
> -__nfs4_find_lock_state(struct nfs4_state *state, fl_owner_t fl_owner)
> +__nfs4_find_lock_state(struct nfs4_state *state,
> + fl_owner_t fl_owner, fl_owner_t fl_owner2)
> {
> - struct nfs4_lock_state *pos;
> + struct nfs4_lock_state *pos, *ret = NULL;
> list_for_each_entry(pos, &state->lock_states, ls_locks) {
> - if (pos->ls_owner != fl_owner)
> - continue;
> - atomic_inc(&pos->ls_count);
> - return pos;
> + if (pos->ls_owner == fl_owner) {
> + ret = pos;
> + break;
> + }
> + if (pos->ls_owner == fl_owner2)
> + ret = pos;
> }
> - return NULL;
> + if (ret)
> + atomic_inc(&ret->ls_count);
> + return ret;
> }
>
> /*
> @@ -857,7 +871,7 @@ static struct nfs4_lock_state *nfs4_get_lock_state(struct nfs4_state *state, fl_
>
> for(;;) {
> spin_lock(&state->state_lock);
> - lsp = __nfs4_find_lock_state(state, owner);
> + lsp = __nfs4_find_lock_state(state, owner, 0);
> if (lsp != NULL)
> break;
> if (new != NULL) {
> @@ -942,7 +956,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
> const struct nfs_lock_context *l_ctx)
> {
> struct nfs4_lock_state *lsp;
> - fl_owner_t fl_owner;
> + fl_owner_t fl_owner, fl_flock_owner;
> int ret = -ENOENT;
>
> if (l_ctx == NULL)
> @@ -952,8 +966,10 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
> goto out;
>
> fl_owner = l_ctx->lockowner.l_owner;
> + fl_flock_owner = l_ctx->open_context->flock_owner;
> +
> spin_lock(&state->state_lock);
> - lsp = __nfs4_find_lock_state(state, fl_owner);
> + lsp = __nfs4_find_lock_state(state, fl_owner, fl_flock_owner);
> if (lsp && test_bit(NFS_LOCK_LOST, &lsp->ls_flags))
> ret = -EIO;
> else if (lsp != NULL && test_bit(NFS_LOCK_INITIALIZED, &lsp->ls_flags) != 0) {
Predictable behavior is even better there, and I agree that picking
POSIX locks over flock/OFD makes more sense for historical reasons.
Reviewed-by: Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH] NFSv4: ensure __nfs4_find_lock_state returns consistent result.
2016-10-14 10:49 ` Jeff Layton
@ 2016-12-19 0:33 ` NeilBrown
0 siblings, 0 replies; 19+ messages in thread
From: NeilBrown @ 2016-12-19 0:33 UTC (permalink / raw)
To: Trond Myklebust
Cc: Jeff Layton, Anna Schumaker, Benjamin Coddington, Linux NFS Mailing List
[-- Attachment #1: Type: text/plain, Size: 2886 bytes --]
If a file has both flock locks and OFD locks, then it is possible that
two different nfs4 lock states could apply to file accesses from a
single process.
It is not possible to know, efficiently, which one is "correct".
Presumably the state which represents a lock that covers the region
undergoing IO would be the "correct" one to use, but finding that has
a non-trivial cost and would provide miniscule value.
Currently we just return whichever is first in the list, which could
result in inconsistent behaviour if an application ever put it self in
this position. As consistent behaviour is preferable (when perfectly
correct behaviour is not available), change the search to return a
consistent result in this circumstance.
Specifically: if there is both a flock and OFD lock state, always return
the flock one.
Reviewed-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
Hi Trond,
thanks for applying my recent lock-state management series.
This patch was missed, probably because it was buried in a reply to a
conversation. Sorry about that.
It is not a critical fix, but it does complete the series.
Thanks,
NeilBrown
fs/nfs/nfs4state.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 95baf7d340f0..cf869802ff23 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -797,21 +797,33 @@ void nfs4_close_sync(struct nfs4_state *state, fmode_t fmode)
/*
* Search the state->lock_states for an existing lock_owner
- * that is compatible with current->files
+ * that is compatible with either of the given owners.
+ * If the second is non-zero, then the first refers to a Posix-lock
+ * owner (current->files) and the second refers to a flock/OFD
+ * owner (struct file*). In that case, prefer a match for the first
+ * owner.
+ * If both sorts of locks are held on the one file we cannot know
+ * which stateid was intended to be used, so a "correct" choice cannot
+ * be made. Failing that, a "consistent" choice is preferable. The
+ * consistent choice we make is to prefer the first owner, that of a
+ * Posix lock.
*/
static struct nfs4_lock_state *
__nfs4_find_lock_state(struct nfs4_state *state,
fl_owner_t fl_owner, fl_owner_t fl_owner2)
{
- struct nfs4_lock_state *pos;
+ struct nfs4_lock_state *pos, *ret = NULL;
list_for_each_entry(pos, &state->lock_states, ls_locks) {
- if (pos->ls_owner != fl_owner &&
- pos->ls_owner != fl_owner2)
- continue;
- atomic_inc(&pos->ls_count);
- return pos;
+ if (pos->ls_owner == fl_owner) {
+ ret = pos;
+ break;
+ }
+ if (pos->ls_owner == fl_owner2)
+ ret = pos;
}
- return NULL;
+ if (ret)
+ atomic_inc(&ret->ls_count);
+ return ret;
}
/*
--
2.11.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2
2016-10-13 4:26 [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2 NeilBrown
` (5 preceding siblings ...)
2016-10-13 4:26 ` [PATCH 5/6] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one NeilBrown
@ 2016-10-13 15:31 ` Jeff Layton
2016-10-18 21:52 ` NeilBrown
6 siblings, 1 reply; 19+ messages in thread
From: Jeff Layton @ 2016-10-13 15:31 UTC (permalink / raw)
To: NeilBrown, Trond Myklebust, Anna Schumaker
Cc: Benjamin Coddington, Linux NFS Mailing List
On Thu, 2016-10-13 at 15:26 +1100, NeilBrown wrote:
> Thanks to prompting from Jeff I discovered that I could
> access the correct open-state when ftrunctate() is being used.
> That improves the setattr handling, removing some special cases.
>
> This series is based on the linux-next branch of
> git://git.linux-nfs.org/projects/anna/linux-nfs.git
>
> NeilBrown
>
> ---
>
> NeilBrown (6):
> NFS: remove l_pid field from nfs_lockowner
> NFSv4: add flock_owner to open context
> NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.
> NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner
> NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one
> NFS: discard nfs_lockowner structure.
>
>
> fs/nfs/dir.c | 6 +++---
> fs/nfs/inode.c | 14 +++++++-------
> fs/nfs/nfs4_fs.h | 2 +-
> fs/nfs/nfs4file.c | 2 +-
> fs/nfs/nfs4proc.c | 44 +++++++++++++++++++-------------------------
> fs/nfs/nfs4state.c | 25 ++++++++++++++-----------
> fs/nfs/pagelist.c | 3 +--
> fs/nfs/write.c | 3 +--
> include/linux/nfs_fs.h | 10 +++-------
> 9 files changed, 50 insertions(+), 59 deletions(-)
>
> --
> Signature
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
This all looks good to me aside from some minor documentation nits.
Kudos too on doing this with a net removal of code. :)
Reviewed-by: Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2
2016-10-13 15:31 ` [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2 Jeff Layton
@ 2016-10-18 21:52 ` NeilBrown
2016-11-18 4:59 ` NeilBrown
0 siblings, 1 reply; 19+ messages in thread
From: NeilBrown @ 2016-10-18 21:52 UTC (permalink / raw)
To: Jeff Layton, Trond Myklebust, Anna Schumaker
Cc: Benjamin Coddington, Linux NFS Mailing List
[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]
On Fri, Oct 14 2016, Jeff Layton wrote:
>
> This all looks good to me aside from some minor documentation nits.
> Kudos too on doing this with a net removal of code. :)
>
> Reviewed-by: Jeff Layton <jlayton@redhat.com>
Thanks.
The full series, with Reviewed-by added, is available as below if
a "git pull" is easier that plucking them out of emails
NeilBrown
The following changes since commit 1001354ca34179f3db924eb66672442a173147dc:
Linux 4.9-rc1 (2016-10-15 12:17:50 -0700)
are available in the git repository at:
git://neil.brown.name/linux tags/nfs-flock-fix
for you to fetch changes up to 077829bfd0e32131b49d87476b69a189e8e13ae8:
NFS: discard nfs_lockowner structure. (2016-10-19 08:44:25 +1100)
----------------------------------------------------------------
Series to fix NFSv4 stateid used for flock locks
----------------------------------------------------------------
NeilBrown (6):
NFS: remove l_pid field from nfs_lockowner
NFSv4: add flock_owner to open context
NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.
NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner
NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one
NFS: discard nfs_lockowner structure.
fs/nfs/dir.c | 6 +++---
fs/nfs/inode.c | 14 +++++++-------
fs/nfs/nfs4_fs.h | 2 +-
fs/nfs/nfs4file.c | 2 +-
fs/nfs/nfs4proc.c | 44 +++++++++++++++++++-------------------------
fs/nfs/nfs4state.c | 49 ++++++++++++++++++++++++++++++++-----------------
fs/nfs/pagelist.c | 3 +--
fs/nfs/write.c | 3 +--
include/linux/nfs_fs.h | 10 +++-------
9 files changed, 68 insertions(+), 65 deletions(-)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2
2016-10-18 21:52 ` NeilBrown
@ 2016-11-18 4:59 ` NeilBrown
0 siblings, 0 replies; 19+ messages in thread
From: NeilBrown @ 2016-11-18 4:59 UTC (permalink / raw)
To: Jeff Layton, Trond Myklebust, Anna Schumaker
Cc: Benjamin Coddington, Linux NFS Mailing List
[-- Attachment #1: Type: text/plain, Size: 2096 bytes --]
Hi all,
what is the status here. I don't see these in linux-next, nor do I see
any issues raise with them.
Is there any chance they can get into 4.10??
Thanks,
NeilBrown
On Wed, Oct 19 2016, NeilBrown wrote:
> [ Unknown signature status ]
> On Fri, Oct 14 2016, Jeff Layton wrote:
>
>>
>> This all looks good to me aside from some minor documentation nits.
>> Kudos too on doing this with a net removal of code. :)
>>
>> Reviewed-by: Jeff Layton <jlayton@redhat.com>
>
> Thanks.
>
> The full series, with Reviewed-by added, is available as below if
> a "git pull" is easier that plucking them out of emails
>
> NeilBrown
>
>
> The following changes since commit 1001354ca34179f3db924eb66672442a173147dc:
>
> Linux 4.9-rc1 (2016-10-15 12:17:50 -0700)
>
> are available in the git repository at:
>
> git://neil.brown.name/linux tags/nfs-flock-fix
>
> for you to fetch changes up to 077829bfd0e32131b49d87476b69a189e8e13ae8:
>
> NFS: discard nfs_lockowner structure. (2016-10-19 08:44:25 +1100)
>
> ----------------------------------------------------------------
> Series to fix NFSv4 stateid used for flock locks
>
> ----------------------------------------------------------------
> NeilBrown (6):
> NFS: remove l_pid field from nfs_lockowner
> NFSv4: add flock_owner to open context
> NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.
> NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner
> NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one
> NFS: discard nfs_lockowner structure.
>
> fs/nfs/dir.c | 6 +++---
> fs/nfs/inode.c | 14 +++++++-------
> fs/nfs/nfs4_fs.h | 2 +-
> fs/nfs/nfs4file.c | 2 +-
> fs/nfs/nfs4proc.c | 44 +++++++++++++++++++-------------------------
> fs/nfs/nfs4state.c | 49 ++++++++++++++++++++++++++++++++-----------------
> fs/nfs/pagelist.c | 3 +--
> fs/nfs/write.c | 3 +--
> include/linux/nfs_fs.h | 10 +++-------
> 9 files changed, 68 insertions(+), 65 deletions(-)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread