NFSd state: nfs4_lock_state() and nfs4_lock_state()

DISCUSSION Stanislav Kinsbursky 2 participants 8 messages Nov 16, 2012 – Nov 27, 2012

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Stanislav Kinsbursky

author

2012-11-16 11:25 UTC #0 lore raw

Hello, guys.
Right now I'm looking how client_mutex can be containerised.
And it looks like this mutex is too widely used. And I can't get what it protects exactly.

I'd like to hear your opinions about the following:
1) Why do we need to use this mutex in nfsd4_load_reboot_recovery_data()? This function is called only once on NFS server start before launching kthreads.

2) Look like using of this mutex can be easely moved out from read, write and setattr functions in nfs4proc.c to nfs4_preprocess_stateid_op() in nfs4state.c. It it also could be removed from nfs4_open(), then nfs4_lock_state() and nfs4_lock_state() can become static.

So the question is: why this mutex covers that much different code in nfsd4_open() call?

Signature · 2 lines

Best regards,
Stanislav Kinsbursky

[email protected]

2012-11-16 16:58 UTC #1 lore raw

Quoted 3 lines · Stanislav Kinsbursky

Hello, guys.
Right now I'm looking how client_mutex can be containerised. And it looks like this mutex is too widely used.

Yep.

Quoted 1 line

And I can't get what it protects exactly.

Nobody does.

Quoted 4 lines

I'd like to hear your opinions about the following:
1) Why do we need to use this mutex in nfsd4_load_reboot_recovery_data()? This function is called only once on NFS server start before launching kthreads.

I agree, that looks unnecessary.

A patch follows: note it's a two-line patch, with 20 lines of changelog showing that I looked at what state might be shared by other threads and explaining why I think this is safe.

I think that's what we need to do: little patches that remove it from one or another part of the code with careful explanation of why it works.

(I'm not completely sure this particular patch really helps reduce the scope of the state lock that much, but maybe it's worth it.)

Quoted 3 lines

2) Look like using of this mutex can be easely moved out from read, write and setattr functions in nfs4proc.c to nfs4_preprocess_stateid_op() in nfs4state.c.

Could be.

Quoted 3 lines

It it also could be
removed from nfs4_open(), then nfs4_lock_state() and nfs4_lock_state() can become static.

I think you'll nfsd4_open is much more complicated.

Quoted 2 lines

So the question is: why this mutex covers that much different code in nfsd4_open() call?

There are a number of problems. As just one example, the state lock is all that guarantees the open owner will survive and not, for example, be freed before we reach the encode_seqid_op_tail in the xdr code (see the

	if (cstate->replay_owner) {
		nfs4_unlock_state();
		cstate->replay_owner = NULL;
	}

at the end of nfsd4_proc_compound).

I'm trying to get the open code fixed, but that project's on hold at the moment.

--b.

commit 8b78ea67502007d2b2a7f5e25aeea47340eceb98

Author: J. Bruce Fields <[email protected]>
Date:   Fri Nov 16 11:45:12 2012 -0500

nfsd4: remove state lock from nfsd4_load_reboot_recovery_data

That function is only called under nfsd_mutex: we know that because the only caller is nfsd_svc, via

nfsd_svc
nfsd_startup
nfs4_state_start
nfsd4_client_tracking_init
client_tracking_ops->init == nfsd4_load_reboot_recovery_data

The shared state accessed here includes:

- user_recovery_dirname: used here, modified only by nfs4_reset_recoverydir, which can be verified to only be called under nfsd_mutex.
- filesystem state, protected by i_mutex (handwaving slightly
here)
- rec_file, reclaim_str_hashtbl, reclaim_str_hashtbl_size: other than here, used only from code called from nfsd or laundromat threads, both of which should be started only after this runs (see nfsd_svc) and stopped before this could run again (see nfsd_shutdown, called from nfsd_last_thread).

Signed-off-by J. Bruce Fields <[email protected]>

fs/nfsd/nfs4recover.c +0 -2

index 43295d4..ad15e77 100644
@@ -424,11 +424,9 @@ nfsd4_load_reboot_recovery_data(struct net *net)
 		return -EINVAL;
 	}
 
-	nfs4_lock_state();
 	status = nfsd4_init_recdir();
 	if (!status)
 		status = nfsd4_recdir_load();
-	nfs4_unlock_state();
 	if (status)
 		printk(KERN_ERR "NFSD: Failure reading reboot recovery data\n");
 	return status;

Stanislav Kinsbursky

author Acked-by

2012-11-19 08:39 UTC #2 lore raw

16.11.2012 20:58, [email protected] пишет:

Quoted 5 lines

A patch follows: note it's a two-line patch, with 20 lines of changelog showing that I looked at what state might be shared by other threads and explaining why I think this is safe.

Acked-by Stanislav Kinsburskiy <[email protected]>

Quoted 4 lines

I think that's what we need to do: little patches that remove it from one or another part of the code with careful explanation of why it works.

Yes, thanks. I'll also try to simplify nfsd_open() by little patches.
In general it looks like client_mutex protect two different things: open owner state (which is containerised already, actually) and files access. So, probably, this client mutex have to be converted into two: per-net one, which protects open owner state, and static one, which protects files operations. What do you think about it?

Signature · 2 lines

Best regards,
Stanislav Kinsbursky

[email protected]

2012-11-19 12:46 UTC #3 lore raw

Quoted 14 lines · Stanislav Kinsbursky

16.11.2012 20:58, [email protected] пишет:
Quoted 5 lines
A patch follows: note it's a two-line patch, with 20 lines of changelog showing that I looked at what state might be shared by other threads and explaining why I think this is safe.
Acked-by Stanislav Kinsburskiy <[email protected]>
Quoted 4 lines
I think that's what we need to do: little patches that remove it from one or another part of the code with careful explanation of why it works.

Quoted 8 lines

Yes, thanks. I'll also try to simplify nfsd_open() by little patches. In general it looks like client_mutex protect two different things:
open owner state (which is containerised already, actually) and files access.
So, probably, this client mutex have to be converted into two:
per-net one, which protects open owner state, and static one, which protects files operations.
What do you think about it?

That sounds right.

--b.

Stanislav Kinsbursky

author

2012-11-23 11:31 UTC #4 lore raw

19.11.2012 16:46, [email protected] пишет:

Quoted 24 lines · Stanislav Kinsbursky

Quoted 14 lines
16.11.2012 20:58, [email protected] пишет:
Quoted 5 lines
A patch follows: note it's a two-line patch, with 20 lines of changelog showing that I looked at what state might be shared by other threads and explaining why I think this is safe.
Acked-by Stanislav Kinsburskiy <[email protected]>
Quoted 4 lines
I think that's what we need to do: little patches that remove it from one or another part of the code with careful explanation of why it works.
Quoted 8 lines
Yes, thanks. I'll also try to simplify nfsd_open() by little patches. In general it looks like client_mutex protect two different things:
open owner state (which is containerised already, actually) and files access.
So, probably, this client mutex have to be converted into two:
per-net one, which protects open owner state, and static one, which protects files operations.
What do you think about it?

Quoted 2 lines

That sounds right.

Looked into the code, and few more questions raised.
So, currently, this mutex protects different NFS4 state structures and VFS file operations (like open, read, write, etc.).
I would like this mutex to be converted into per-net one. Since NFS4 state structures are per-net already (with my previous patch set), the only thing left is VFS operations. But do we really need to protect them somehow from concurrent access?
I.e. do we have some RPCs, which does more that one file manipulation, but this manipulations sequence have to be "atomic" in NFSd terms?

Signature · 2 lines

Best regards,
Stanislav Kinsbursky

[email protected]

2012-11-26 22:36 UTC #5 lore raw

Quoted 28 lines · Stanislav Kinsbursky

19.11.2012 16:46, [email protected] пишет:
Quoted 24 lines · Stanislav Kinsbursky
Quoted 14 lines
16.11.2012 20:58, [email protected] пишет:
Quoted 5 lines
A patch follows: note it's a two-line patch, with 20 lines of changelog showing that I looked at what state might be shared by other threads and explaining why I think this is safe.
Acked-by Stanislav Kinsburskiy <[email protected]>
Quoted 4 lines
I think that's what we need to do: little patches that remove it from one or another part of the code with careful explanation of why it works.
Quoted 8 lines
Yes, thanks. I'll also try to simplify nfsd_open() by little patches. In general it looks like client_mutex protect two different things:
open owner state (which is containerised already, actually) and files access.
So, probably, this client mutex have to be converted into two:
per-net one, which protects open owner state, and static one, which protects files operations.
What do you think about it?
Quoted 2 lines
That sounds right.

Quoted 7 lines

Looked into the code, and few more questions raised.
So, currently, this mutex protects different NFS4 state structures and VFS file operations (like open, read, write, etc.). I would like this mutex to be converted into per-net one. Since NFS4
state structures are per-net already (with my previous patch set),
the only thing left is VFS operations.
But do we really need to protect them somehow from concurrent access?
I.e. do we have some RPCs, which does more that one file manipulation, but this manipulations sequence have to be "atomic" in NFSd terms?

Probably not, but open is the most likely problem, as it does several things which are not all atomic from the point of view of the vfs. The mutex can't prevent races between an nfs client and a local user, but it can at least prevent races between nfs clients.

--b.

Stanislav Kinsbursky

author

2012-11-27 07:54 UTC #6 lore raw

27.11.2012 02:36, [email protected] пишет:

Quoted 37 lines · Stanislav Kinsbursky

Quoted 28 lines
19.11.2012 16:46, [email protected] пишет:
Quoted 24 lines · Stanislav Kinsbursky
Quoted 14 lines
16.11.2012 20:58, [email protected] пишет:
Quoted 5 lines
A patch follows: note it's a two-line patch, with 20 lines of changelog showing that I looked at what state might be shared by other threads and explaining why I think this is safe.
Acked-by Stanislav Kinsburskiy <[email protected]>
Quoted 4 lines
I think that's what we need to do: little patches that remove it from one or another part of the code with careful explanation of why it works.
Quoted 8 lines
Yes, thanks. I'll also try to simplify nfsd_open() by little patches. In general it looks like client_mutex protect two different things:
open owner state (which is containerised already, actually) and files access.
So, probably, this client mutex have to be converted into two:
per-net one, which protects open owner state, and static one, which protects files operations.
What do you think about it?
Quoted 2 lines
That sounds right.
Quoted 7 lines
Looked into the code, and few more questions raised.
So, currently, this mutex protects different NFS4 state structures and VFS file operations (like open, read, write, etc.). I would like this mutex to be converted into per-net one. Since NFS4
state structures are per-net already (with my previous patch set),
the only thing left is VFS operations.
But do we really need to protect them somehow from concurrent access?
I.e. do we have some RPCs, which does more that one file manipulation, but this manipulations sequence have to be "atomic" in NFSd terms?

Quoted 5 lines

Probably not, but open is the most likely problem, as it does several things which are not all atomic from the point of view of the vfs. The mutex can't prevent races between an nfs client and a local user, but it can at least prevent races between nfs clients.

Ok then.
You said, that you are going to optimize open a bit?
I think client_mutex containerization can be delayed for a while. Thnaks.

Quoted 2 lines

--b.

Signature · 2 lines

Best regards,
Stanislav Kinsbursky

[email protected]

2012-11-27 22:09 UTC #7 lore raw

Quoted 38 lines · Stanislav Kinsbursky

27.11.2012 02:36, [email protected] пишет:
Quoted 37 lines · Stanislav Kinsbursky
Quoted 35 lines
19.11.2012 16:46, [email protected] пишет:
Quoted 24 lines · Stanislav Kinsbursky
Quoted 14 lines
16.11.2012 20:58, [email protected] пишет:
Quoted 5 lines
A patch follows: note it's a two-line patch, with 20 lines of changelog showing that I looked at what state might be shared by other threads and explaining why I think this is safe.
Acked-by Stanislav Kinsburskiy <[email protected]>
Quoted 4 lines
I think that's what we need to do: little patches that remove it from one or another part of the code with careful explanation of why it works.
Quoted 8 lines
Yes, thanks. I'll also try to simplify nfsd_open() by little patches. In general it looks like client_mutex protect two different things:
open owner state (which is containerised already, actually) and files access.
So, probably, this client mutex have to be converted into two:
per-net one, which protects open owner state, and static one, which protects files operations.
What do you think about it?
Quoted 2 lines
That sounds right.
Looked into the code, and few more questions raised.
So, currently, this mutex protects different NFS4 state structures and VFS file operations (like open, read, write, etc.). I would like this mutex to be converted into per-net one. Since NFS4
state structures are per-net already (with my previous patch set),
the only thing left is VFS operations.
But do we really need to protect them somehow from concurrent access?
I.e. do we have some RPCs, which does more that one file manipulation, but this manipulations sequence have to be "atomic" in NFSd terms?

Quoted 8 lines

Quoted 5 lines
Probably not, but open is the most likely problem, as it does several things which are not all atomic from the point of view of the vfs. The mutex can't prevent races between an nfs client and a local user, but it can at least prevent races between nfs clients.
Ok then.
You said, that you are going to optimize open a bit?

It's on my todo list, but I'm not sure when it will be done.

Quoted 1 line

I think client_mutex containerization can be delayed for a while.

OK.--b.