[lustre-devel] [PATCH 32/38] lustre: osc: restore cl_loi_list_lock

[Andreas Dilger]

On Aug 17, 2018, at 18:59, James Simmons <jsimmons at infradead.org> wrote:
> 
> 
>>> Access to struct client_obd should be protected with the spinlock
>>> cl_loi_list_lock. This was dropped during the port to sysfs so
>>> restore the proper locking.
>>> 
>>> Signed-off-by: James Simmons <jsimmons at infradead.org>
>>> ---
>>> drivers/staging/lustre/lustre/osc/lproc_osc.c | 36 +++++++++++++++++++++++----
>>> 1 file changed, 31 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c b/drivers/staging/lustre/lustre/osc/lproc_osc.c
>>> index 3c31e98..5fb7a16 100644
>>> --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
>>> +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
>>> @@ -80,8 +80,13 @@ static ssize_t max_rpcs_in_flight_show(struct kobject *kobj,
>>> 	struct obd_device *dev = container_of(kobj, struct obd_device,
>>> 					      obd_kset.kobj);
>>> 	struct client_obd *cli = &dev->u.cli;
>>> +	ssize_t len;
>>> 
>>> -	return sprintf(buf, "%u\n", cli->cl_max_rpcs_in_flight);
>>> +	spin_lock(&cli->cl_loi_list_lock);
>>> +	len = sprintf(buf, "%u\n", cli->cl_max_rpcs_in_flight);
>>> +	spin_unlock(&cli->cl_loi_list_lock);
>> 
>> Why do you think a spinlock is needed here?
>> How could you even end up with an incorrect value?
> 
> I see both mdc sysfs and the osc sysfs layer being able to
> modify cl_max_rpcs_in_flight. Andreas is the struct client_obd
> shared between both the osc and mdc layer or does each subsystem
> contain a unique cli in struct obd_device?

Each OBD device (OSC, MDC, MGC, etc) has its own struct obd_device
with an embedded struct client_obd, so it is not shared between the
OSC and MDC.  These tunables could potentially be set differently
for each filesystem, or even for each target within a single filesystem
(e.g. if some flash OSTs need more RPCs in flight to keep busy than
HDD-based OSTs).

Since this is just printing the value, it doesn't make sense to have
locking in any case.  At worst the cl_max_rpcs_in_flight value used
may ne nanoseconds out of date when accessed, and microseconds old
when printed.  That race exists whether the locking is here or not
(i.e. the value could be changed immediately after it is printed into
the buffer and the lock is dropped).

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud







-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 235 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20180818/7a46220c/attachment.sig>