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