| Message ID | 1534475441-15543-33-git-send-email-jsimmons@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lustre: fixes for sysfs handling | expand |
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
> > 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?
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
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
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);
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(-)