[32/38] lustre: osc: restore cl_loi_list_lock
diff mbox series

Message ID 1534475441-15543-33-git-send-email-jsimmons@infradead.org
State New
Headers show
Series
  • lustre: fixes for sysfs handling
Related show

Commit Message

James Simmons Aug. 17, 2018, 3:10 a.m. UTC
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@infradead.org>
---
 drivers/staging/lustre/lustre/osc/lproc_osc.c | 36 +++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 5 deletions(-)

Comments

NeilBrown Aug. 17, 2018, 7:30 a.m. UTC | #1
On Thu, Aug 16 2018, James Simmons 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@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?

NeilBrown
James Simmons Aug. 18, 2018, 12:59 a.m. UTC | #2
> > 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@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?
Andreas Dilger Aug. 18, 2018, 5:08 a.m. UTC | #3
On Aug 17, 2018, at 18:59, James Simmons <jsimmons@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@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
Andreas Dilger Aug. 18, 2018, 5:10 a.m. UTC | #4
On Aug 17, 2018, at 23:08, Andreas Dilger <adilger@whamcloud.com> wrote:
> 
> Signed PGP part
> On Aug 17, 2018, at 18:59, James Simmons <jsimmons@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@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).

The only reason to keep this locking might be to make static code
analysis tools happy, so that they always see cl_max_rpcs_in_flight
access protected by a spinlock, instead of inconsistent locking.

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

Patch
diff mbox series

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);
+
+	return len;
 }
 
 static ssize_t max_rpcs_in_flight_store(struct kobject *kobj,
@@ -136,7 +141,10 @@  static ssize_t max_dirty_mb_show(struct kobject *kobj,
 	long val;
 	int mult;
 
+	spin_lock(&cli->cl_loi_list_lock);
 	val = cli->cl_dirty_max_pages;
+	spin_unlock(&cli->cl_loi_list_lock);
+
 	mult = 1 << (20 - PAGE_SHIFT);
 	return lprocfs_read_frac_helper(buf, PAGE_SIZE, val, mult);
 }
@@ -247,9 +255,13 @@  static ssize_t cur_dirty_bytes_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, "%lu\n", cli->cl_dirty_pages << PAGE_SHIFT);
+	spin_lock(&cli->cl_loi_list_lock);
+	len = sprintf(buf, "%lu\n", cli->cl_dirty_pages << PAGE_SHIFT);
+	spin_unlock(&cli->cl_loi_list_lock);
 
+	return len;
 }
 LUSTRE_RO_ATTR(cur_dirty_bytes);
 
@@ -260,8 +272,13 @@  static ssize_t cur_grant_bytes_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, "%lu\n", cli->cl_avail_grant);
+	spin_lock(&cli->cl_loi_list_lock);
+	len = sprintf(buf, "%lu\n", cli->cl_avail_grant);
+	spin_unlock(&cli->cl_loi_list_lock);
+
+	return len;
 }
 
 static ssize_t cur_grant_bytes_store(struct kobject *kobj,
@@ -280,8 +297,12 @@  static ssize_t cur_grant_bytes_store(struct kobject *kobj,
 		return rc;
 
 	/* this is only for shrinking grant */
-	if (val >= cli->cl_avail_grant)
+	spin_lock(&cli->cl_loi_list_lock);
+	if (val >= cli->cl_avail_grant) {
+		spin_unlock(&cli->cl_loi_list_lock);
 		return -EINVAL;
+	}
+	spin_unlock(&cli->cl_loi_list_lock);
 
 	if (cli->cl_import->imp_state == LUSTRE_IMP_FULL)
 		rc = osc_shrink_grant_to_target(cli, val);
@@ -298,8 +319,13 @@  static ssize_t cur_lost_grant_bytes_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, "%lu\n", cli->cl_lost_grant);
+	spin_lock(&cli->cl_loi_list_lock);
+	len = sprintf(buf, "%lu\n", cli->cl_lost_grant);
+	spin_unlock(&cli->cl_loi_list_lock);
+
+	return len;
 }
 LUSTRE_RO_ATTR(cur_lost_grant_bytes);