Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 8003d3c4 authored by Jeff Layton's avatar Jeff Layton Committed by Trond Myklebust
Browse files

nfs4: treat lock owners as opaque values 



Do the following set of ops with a file on a NFSv4 mount:

    exec 3>>/file/on/nfsv4
    flock -x 3
    exec 3>&-

You'll see the LOCK request go across the wire, but no LOCKU when the
file is closed.

What happens is that the fd is passed across a fork, and the final close
is done in a different process than the opener. That makes
__nfs4_find_lock_state miss finding the correct lock state because it
uses the fl_pid as a search key. A new one is created, and the locking
code treats it as a delegation stateid (because NFS_LOCK_INITIALIZED
isn't set).

The root cause of this breakage seems to be commit 77041ed9
(NFSv4: Ensure the lockowners are labelled using the fl_owner and/or
fl_pid).

That changed it so that flock lockowners are allocated based on the
fl_pid. I think this is incorrect. flock locks should be "owned" by the
struct file, and that is already accounted for in the fl_owner field of
the lock request when it comes through nfs_flock.

This patch basically reverts the above commit and with it, a LOCKU is
sent in the above reproducer.

Signed-off-by: default avatarJeff Layton <jlayton@poochiereds.net>
Signed-off-by: default avatarTrond Myklebust <trond.myklebust@primarydata.com>
parent 039b756a

fs/nfs/nfs4_fs.h

+1 −12
Original line number Diff line number Diff line
@@ -129,17 +129,6 @@ enum { 
 * LOCK: one nfs4_state (LOCK) to hold the lock stateid nfs4_state(OPEN)

 */



struct nfs4_lock_owner {

	unsigned int lo_type;

#define NFS4_ANY_LOCK_TYPE	(0U)

#define NFS4_FLOCK_LOCK_TYPE	(1U << 0)

#define NFS4_POSIX_LOCK_TYPE	(1U << 1)

	union {

		fl_owner_t posix_owner;

		pid_t flock_owner;

	} lo_u;

};



struct nfs4_lock_state {

	struct list_head	ls_locks;	/* Other lock stateids */

	struct nfs4_state *	ls_state;	/* Pointer to open state */
@@ -149,7 +138,7 @@ struct nfs4_lock_state { 
	struct nfs_seqid_counter	ls_seqid;

	nfs4_stateid		ls_stateid;

	atomic_t		ls_count;

	struct nfs4_lock_owner	ls_owner;

	fl_owner_t		ls_owner;

};



/* bits for nfs4_state->flags */

fs/nfs/nfs4state.c

+9 −36
Original line number Diff line number Diff line
@@ -787,21 +787,12 @@ 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, pid_t fl_pid, unsigned int type)

__nfs4_find_lock_state(struct nfs4_state *state, fl_owner_t fl_owner)

{

	struct nfs4_lock_state *pos;

	list_for_each_entry(pos, &state->lock_states, ls_locks) {

		if (type != NFS4_ANY_LOCK_TYPE && pos->ls_owner.lo_type != type)

		if (pos->ls_owner != fl_owner)

			continue;

		switch (pos->ls_owner.lo_type) {

		case NFS4_POSIX_LOCK_TYPE:

			if (pos->ls_owner.lo_u.posix_owner != fl_owner)

				continue;

			break;

		case NFS4_FLOCK_LOCK_TYPE:

			if (pos->ls_owner.lo_u.flock_owner != fl_pid)

				continue;

		}

		atomic_inc(&pos->ls_count);

		return pos;

	}
@@ -813,7 +804,7 @@ __nfs4_find_lock_state(struct nfs4_state *state, fl_owner_t fl_owner, pid_t fl_p 
 * exists, return an uninitialized one.

 *

 */

static struct nfs4_lock_state *nfs4_alloc_lock_state(struct nfs4_state *state, fl_owner_t fl_owner, pid_t fl_pid, unsigned int type)

static struct nfs4_lock_state *nfs4_alloc_lock_state(struct nfs4_state *state, fl_owner_t fl_owner)

{

	struct nfs4_lock_state *lsp;

	struct nfs_server *server = state->owner->so_server;
@@ -824,17 +815,7 @@ static struct nfs4_lock_state *nfs4_alloc_lock_state(struct nfs4_state *state, f 
	nfs4_init_seqid_counter(&lsp->ls_seqid);

	atomic_set(&lsp->ls_count, 1);

	lsp->ls_state = state;

	lsp->ls_owner.lo_type = type;

	switch (lsp->ls_owner.lo_type) {

	case NFS4_FLOCK_LOCK_TYPE:

		lsp->ls_owner.lo_u.flock_owner = fl_pid;

		break;

	case NFS4_POSIX_LOCK_TYPE:

		lsp->ls_owner.lo_u.posix_owner = fl_owner;

		break;

	default:

		goto out_free;

	}

	lsp->ls_owner = fl_owner;

	lsp->ls_seqid.owner_id = ida_simple_get(&server->lockowner_id, 0, 0, GFP_NOFS);

	if (lsp->ls_seqid.owner_id < 0)

		goto out_free;
@@ -857,13 +838,13 @@ void nfs4_free_lock_state(struct nfs_server *server, struct nfs4_lock_state *lsp 
 * exists, return an uninitialized one.

 *

 */

static struct nfs4_lock_state *nfs4_get_lock_state(struct nfs4_state *state, fl_owner_t owner, pid_t pid, unsigned int type)

static struct nfs4_lock_state *nfs4_get_lock_state(struct nfs4_state *state, fl_owner_t owner)

{

	struct nfs4_lock_state *lsp, *new = NULL;

	

	for(;;) {

		spin_lock(&state->state_lock);

		lsp = __nfs4_find_lock_state(state, owner, pid, type);

		lsp = __nfs4_find_lock_state(state, owner);

		if (lsp != NULL)

			break;

		if (new != NULL) {
@@ -874,7 +855,7 @@ static struct nfs4_lock_state *nfs4_get_lock_state(struct nfs4_state *state, fl_ 
			break;

		}

		spin_unlock(&state->state_lock);

		new = nfs4_alloc_lock_state(state, owner, pid, type);

		new = nfs4_alloc_lock_state(state, owner);

		if (new == NULL)

			return NULL;

	}
@@ -935,13 +916,7 @@ int nfs4_set_lock_state(struct nfs4_state *state, struct file_lock *fl) 


	if (fl->fl_ops != NULL)

		return 0;

	if (fl->fl_flags & FL_POSIX)

		lsp = nfs4_get_lock_state(state, fl->fl_owner, 0, NFS4_POSIX_LOCK_TYPE);

	else if (fl->fl_flags & FL_FLOCK)

		lsp = nfs4_get_lock_state(state, NULL, fl->fl_pid,

				NFS4_FLOCK_LOCK_TYPE);

	else

		return -EINVAL;

	lsp = nfs4_get_lock_state(state, fl->fl_owner);

	if (lsp == NULL)

		return -ENOMEM;

	fl->fl_u.nfs4_fl.owner = lsp;
@@ -955,7 +930,6 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst, 
{

	struct nfs4_lock_state *lsp;

	fl_owner_t fl_owner;

	pid_t fl_pid;

	int ret = -ENOENT;
@@ -966,9 +940,8 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst, 
		goto out;



	fl_owner = lockowner->l_owner;

	fl_pid = lockowner->l_pid;

	spin_lock(&state->state_lock);

	lsp = __nfs4_find_lock_state(state, fl_owner, fl_pid, NFS4_ANY_LOCK_TYPE);

	lsp = __nfs4_find_lock_state(state, fl_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) {