> On Thu, Nov 3, 2016 at 1:05 AM, Dilger, Andreas <andreas.dilger@xxxxxxxxx> wrote: > 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 > > Thanks Andreas! Will start looking into it. Just to let you know I opened a ticket for you. https://jira.hpdd.intel.com/browse/LU-8802 This way wwe can track the progress and have Lustre developers assigened to look at your work. Thanks for stepping forward. > -- > Kind Regards, > Aya Saif El-yazal Mahfouz > > >> 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
