On Oct 25, 2016, at 10:47, Aya Mahfouz <mahfouz.saif.elyazal@xxxxxxxxx> wrote:
>
> On Mon, Oct 17, 2016 at 10:38:31PM +0000, Dilger, Andreas wrote:
>> On Oct 17, 2016, at 15:46, Aya Mahfouz <mahfouz.saif.elyazal@xxxxxxxxx> wrote:
>>>
>>> class_devno_max is an inline function that returns
>>> MAX_OBD_DEVICES. Replace all calls to the function
>>> by MAX_OBD_DEVICES.
>>
>> Thanks for your patch, but unfortunately it can't be accepted.
>>
>> This function was added in preparation of being able to tune the maximum
>> number of storage devices dynamically, rather than having to hard code it
>> to the maximum possible number of servers that a client can possibly
>> connect to.
>>
>> While the current maximum of 8192 servers has been enough for current
>> filesystems, I'd rather move in the direction of dynamically handling this
>> limit rather than re-introducing a hard-coded constant throughout the code.
>>
> Hello,
>
> I would like to proceed with implementing the function if possible.
> Kindly direct me to some starting pointers.
Hi Aya,
thanks for offering to look into this.
There are several ways to approach this problem to make the allocation
of the obd_devs[] array dynamic. In most cases, there isn't any value
to dynamically shrink this array, since the filesystem(s) will typically
be mounted until the node is rebooted, and it is only in the tens of KB
size range, so this will not affect ongoing operations, and that simplifies
the implementation.
The easiest way would be to have a dynamically-sized obd_devs[] array that
is reallocated in class_newdev() in PAGE_SIZE chunks whenever the current
array has no more free slots and copied to the new array, using obd_dev_lock
to protect the array while it is being reallocated and copied. In most
cases, this would save memory over the static array (not many filesystems
have so many servers), but for the few sites that have 10000+ servers they
don't need to change the source to handle this. Using libcfs_kvzalloc()
would avoid issues with allocating large chunks of memory.
There are a few places where obd_devs[] is accessed outside obd_dev_lock
that would need to be fixed now that this array may be changed at runtime.
A second approach that may scale better is to change obd_devs from an array
to a doubly linked list (using standard list_head helpers). In many cases
the whole list is seached linearly, and most of the uses of class_num2obd()
are just used to walk that list in order, which could be replaced with
list_for_each_entry() list traversal. The class_name2dev() function should
be changed to return the pointer to the obd_device structure, and a new
helper class_dev2num() would just return the obd_minor number from the
obd_device struct for the one use in class_resolve_dev_name(). Using a
linked list has the advantage that there is no need to search for free slots
in the array, since devices would be removed from the list when it is freed.
Cheers, Andreas
>> One comment inline below, if you still want to submit a patch.
>>
>>> Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@xxxxxxxxx>
>>> ---
>>> drivers/staging/lustre/lustre/obdclass/class_obd.c | 6 +++---
>>> drivers/staging/lustre/lustre/obdclass/genops.c | 22 +++++++++++-----------
>>> .../lustre/lustre/obdclass/linux/linux-module.c | 6 +++---
>>> 3 files changed, 17 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/staging/lustre/lustre/obdclass/class_obd.c b/drivers/staging/lustre/lustre/obdclass/class_obd.c
>>> index 2b21675..b775c74 100644
>>> --- a/drivers/staging/lustre/lustre/obdclass/class_obd.c
>>> +++ b/drivers/staging/lustre/lustre/obdclass/class_obd.c
>>> @@ -345,7 +345,7 @@ int class_handle_ioctl(unsigned int cmd, unsigned long arg)
>>> goto out;
>>> }
>>> obd = class_name2obd(data->ioc_inlbuf4);
>>> - } else if (data->ioc_dev < class_devno_max()) {
>>> + } else if (data->ioc_dev < MAX_OBD_DEVICES) {
>>> obd = class_num2obd(data->ioc_dev);
>>> } else {
>>> CERROR("OBD ioctl: No device\n");
>>> @@ -498,7 +498,7 @@ static int __init obdclass_init(void)
>>> }
>>>
>>> /* This struct is already zeroed for us (static global) */
>>> - for (i = 0; i < class_devno_max(); i++)
>>> + for (i = 0; i < MAX_OBD_DEVICES; i++)
>>> obd_devs[i] = NULL;
>>
>> This block can just be removed entirely. It used to do something useful,
>> but through a series of changes it has become useless.
>>
>> Cheers, Andreas
>>
>>> /* Default the dirty page cache cap to 1/2 of system memory.
>>> @@ -548,7 +548,7 @@ static void obdclass_exit(void)
>>> lustre_unregister_fs();
>>>
>>> misc_deregister(&obd_psdev);
>>> - for (i = 0; i < class_devno_max(); i++) {
>>> + for (i = 0; i < MAX_OBD_DEVICES; i++) {
>>> struct obd_device *obd = class_num2obd(i);
>>>
>>> if (obd && obd->obd_set_up &&
>>> diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
>>> index 99c2da6..af4fc58 100644
>>> --- a/drivers/staging/lustre/lustre/obdclass/genops.c
>>> +++ b/drivers/staging/lustre/lustre/obdclass/genops.c
>>> @@ -290,7 +290,7 @@ struct obd_device *class_newdev(const char *type_name, const char *name)
>>> LASSERT(newdev->obd_magic == OBD_DEVICE_MAGIC);
>>>
>>> write_lock(&obd_dev_lock);
>>> - for (i = 0; i < class_devno_max(); i++) {
>>> + for (i = 0; i < MAX_OBD_DEVICES; i++) {
>>> struct obd_device *obd = class_num2obd(i);
>>>
>>> if (obd && (strcmp(name, obd->obd_name) == 0)) {
>>> @@ -322,9 +322,9 @@ struct obd_device *class_newdev(const char *type_name, const char *name)
>>> }
>>> write_unlock(&obd_dev_lock);
>>>
>>> - if (!result && i >= class_devno_max()) {
>>> + if (!result && i >= MAX_OBD_DEVICES) {
>>> CERROR("all %u OBD devices used, increase MAX_OBD_DEVICES\n",
>>> - class_devno_max());
>>> + MAX_OBD_DEVICES);
>>> result = ERR_PTR(-EOVERFLOW);
>>> goto out;
>>> }
>>> @@ -372,7 +372,7 @@ int class_name2dev(const char *name)
>>> return -1;
>>>
>>> read_lock(&obd_dev_lock);
>>> - for (i = 0; i < class_devno_max(); i++) {
>>> + for (i = 0; i < MAX_OBD_DEVICES; i++) {
>>> struct obd_device *obd = class_num2obd(i);
>>>
>>> if (obd && strcmp(name, obd->obd_name) == 0) {
>>> @@ -397,7 +397,7 @@ struct obd_device *class_name2obd(const char *name)
>>> {
>>> int dev = class_name2dev(name);
>>>
>>> - if (dev < 0 || dev > class_devno_max())
>>> + if (dev < 0 || dev > MAX_OBD_DEVICES)
>>> return NULL;
>>> return class_num2obd(dev);
>>> }
>>> @@ -408,7 +408,7 @@ int class_uuid2dev(struct obd_uuid *uuid)
>>> int i;
>>>
>>> read_lock(&obd_dev_lock);
>>> - for (i = 0; i < class_devno_max(); i++) {
>>> + for (i = 0; i < MAX_OBD_DEVICES; i++) {
>>> struct obd_device *obd = class_num2obd(i);
>>>
>>> if (obd && obd_uuid_equals(uuid, &obd->obd_uuid)) {
>>> @@ -435,7 +435,7 @@ struct obd_device *class_num2obd(int num)
>>> {
>>> struct obd_device *obd = NULL;
>>>
>>> - if (num < class_devno_max()) {
>>> + if (num < MAX_OBD_DEVICES) {
>>> obd = obd_devs[num];
>>> if (!obd)
>>> return NULL;
>>> @@ -463,7 +463,7 @@ struct obd_device *class_find_client_obd(struct obd_uuid *tgt_uuid,
>>> int i;
>>>
>>> read_lock(&obd_dev_lock);
>>> - for (i = 0; i < class_devno_max(); i++) {
>>> + for (i = 0; i < MAX_OBD_DEVICES; i++) {
>>> struct obd_device *obd = class_num2obd(i);
>>>
>>> if (!obd)
>>> @@ -496,13 +496,13 @@ struct obd_device *class_devices_in_group(struct obd_uuid *grp_uuid, int *next)
>>>
>>> if (!next)
>>> i = 0;
>>> - else if (*next >= 0 && *next < class_devno_max())
>>> + else if (*next >= 0 && *next < MAX_OBD_DEVICES)
>>> i = *next;
>>> else
>>> return NULL;
>>>
>>> read_lock(&obd_dev_lock);
>>> - for (; i < class_devno_max(); i++) {
>>> + for (; i < MAX_OBD_DEVICES; i++) {
>>> struct obd_device *obd = class_num2obd(i);
>>>
>>> if (!obd)
>>> @@ -533,7 +533,7 @@ int class_notify_sptlrpc_conf(const char *fsname, int namelen)
>>> LASSERT(namelen > 0);
>>>
>>> read_lock(&obd_dev_lock);
>>> - for (i = 0; i < class_devno_max(); i++) {
>>> + for (i = 0; i < MAX_OBD_DEVICES; i++) {
>>> obd = class_num2obd(i);
>>>
>>> if (!obd || obd->obd_set_up == 0 || obd->obd_stopping)
>>> diff --git a/drivers/staging/lustre/lustre/obdclass/linux/linux-module.c b/drivers/staging/lustre/lustre/obdclass/linux/linux-module.c
>>> index 33342bf..ca5b466 100644
>>> --- a/drivers/staging/lustre/lustre/obdclass/linux/linux-module.c
>>> +++ b/drivers/staging/lustre/lustre/obdclass/linux/linux-module.c
>>> @@ -228,7 +228,7 @@ static ssize_t health_show(struct kobject *kobj, struct attribute *attr,
>>> return sprintf(buf, "LBUG\n");
>>>
>>> read_lock(&obd_dev_lock);
>>> - for (i = 0; i < class_devno_max(); i++) {
>>> + for (i = 0; i < MAX_OBD_DEVICES; i++) {
>>> struct obd_device *obd;
>>>
>>> obd = class_num2obd(i);
>>> @@ -326,7 +326,7 @@ static struct attribute *lustre_attrs[] = {
>>>
>>> static void *obd_device_list_seq_start(struct seq_file *p, loff_t *pos)
>>> {
>>> - if (*pos >= class_devno_max())
>>> + if (*pos >= MAX_OBD_DEVICES)
>>> return NULL;
>>>
>>> return pos;
>>> @@ -339,7 +339,7 @@ static void obd_device_list_seq_stop(struct seq_file *p, void *v)
>>> static void *obd_device_list_seq_next(struct seq_file *p, void *v, loff_t *pos)
>>> {
>>> ++*pos;
>>> - if (*pos >= class_devno_max())
>>> + if (*pos >= MAX_OBD_DEVICES)
>>> return NULL;
>>>
>>> return pos;
>>> --
>>> 2.5.0
>>>
>>>
>>> --
>>> Kind Regards,
>>> Aya Saif El-yazal Mahfouz
>>> _______________________________________________
>>> lustre-devel mailing list
>>> lustre-devel@xxxxxxxxxxxxxxxx
>>> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
>>
>
> --
> Kind Regards,
> Aya Saif El-yazal Mahfouz
> _______________________________________________
> lustre-devel mailing list
> lustre-devel@xxxxxxxxxxxxxxxx
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
