[dm-devel] [RFC PATCH 17/20] libmultipath/foreign: nvme foreign library
Martin Wilck
mwilck at suse.com
Fri Mar 2 16:04:28 UTC 2018
On Wed, 2018-02-28 at 21:14 -0600, Benjamin Marzinski wrote:
> On Tue, Feb 20, 2018 at 02:26:55PM +0100, Martin Wilck wrote:
> > This still contains stubs for path handling and checking, but it's
> > functional
> > for printing already.
> >
> > Signed-off-by: Martin Wilck <mwilck at suse.com>
> > ---
> > Makefile | 1 +
> > libmultipath/foreign/Makefile | 30 +++
> > libmultipath/foreign/nvme.c | 444
> > ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 475 insertions(+)
> > create mode 100644 libmultipath/foreign/Makefile
> > create mode 100644 libmultipath/foreign/nvme.c
> >
> > diff --git a/libmultipath/foreign/nvme.c
> > b/libmultipath/foreign/nvme.c
> > new file mode 100644
> > index 000000000000..4e9c3a52d03c
> > --- /dev/null
> > +++ b/libmultipath/foreign/nvme.c
> >
> > +static void cleanup_nvme_map(struct nvme_map *map)
> > +{
> > + if (map->udev)
> > + udev_device_unref(map->udev);
> > + if (map->subsys)
> > + udev_device_unref(map->subsys);
>
> According to the libudev reference manual, udev devices returned by
> udev_device_get_parent_with_subsystem_devtype are attached to their
> child and free along with them, so this unref shouldn't be necessary
I'd figured that myself and fixed it in patch 20/20. It must have
excaped my attention when I cleaned up. I'll fix the sequence.
> +
> > +void cleanup(struct context *ctx)
> > +{
> > + if (ctx == NULL)
> > + return;
> > +
> > + (void)delete_all(ctx);
> > +
> > + lock(ctx);
> > + pthread_cleanup_push(unlock, ctx);
> > + vector_free(ctx->mpvec);
> > + pthread_cleanup_pop(1);
> > + pthread_mutex_destroy(&ctx->mutex);
> > +
> > + free(ctx);
> > +}
>
> Would you mind either removing the locking, or adding a comment
> explaining that it's not necessary here. If you really did need to
> lock
> ctx in order guard against someone accessing ctx->mpvec in cleanup(),
> then not setting it to NULL after its freed, and immediately freeing
> ctx
> afterwards would clearly be broken, so seeing the locking makes it
> look
> like this function is wrong.
I don't understand, sorry. I need to lock because some other entity may
still be using ctx->mpvec when cleanup() is called, and I should wait
until that other entity has released the lock before I free it. I'm
also pretty sure that checkers such as coverity would complain if I
free a structure here without locking which I access only under the
lock in other places.
I agree, though, that I should nullify the data and add checks in the
other functions. I'll also add some locking in the foreign.c file,
didn't occur to me before.
> > +static int _add_map(struct context *ctx, struct udev_device *ud,
> > + struct udev_device *subsys)
> > +{
> > + dev_t devt = udev_device_get_devnum(ud);
> > + struct nvme_map *map;
> > +
> > + if (_find_nvme_map_by_devt(ctx, devt) != NULL)
> > + return FOREIGN_OK;
> > +
> > + map = calloc(1, sizeof(*map));
> > + if (map == NULL)
> > + return FOREIGN_ERR;
> > +
> > + map->devt = devt;
> > + map->udev = udev_device_ref(ud);
> > + map->subsys = udev_device_ref(subsys);
>
> You shouldn't need to get a reference here, since map->udev will keep
> this device around.
See above.
Regards,
Martin
>
> -Ben
>
> > + map->gen.ops = &nvme_map_ops;
> > +
> > + if (vector_alloc_slot(ctx->mpvec) == NULL) {
> > + cleanup_nvme_map(map);
> > + return FOREIGN_ERR;
> > + }
> > +
> > + vector_set_slot(ctx->mpvec, map);
> > +
> > + return FOREIGN_CLAIMED;
> > +}
> > +
> > +int add(struct context *ctx, struct udev_device *ud)
> > +{
> > + struct udev_device *subsys;
> > + int rc;
> > +
> > + condlog(5, "%s called for \"%s\"", __func__, THIS);
> > +
> > + if (ud == NULL)
> > + return FOREIGN_ERR;
> > + if (strcmp("disk", udev_device_get_devtype(ud)))
> > + return FOREIGN_IGNORED;
> > +
> > + subsys = udev_device_get_parent_with_subsystem_devtype(ud,
> > + "nv
> > me-subsystem",
> > + NUL
> > L);
> > + if (subsys == NULL)
> > + return FOREIGN_IGNORED;
> > +
> > + lock(ctx);
> > + pthread_cleanup_push(unlock, ctx);
> > + rc = _add_map(ctx, ud, subsys);
> > + pthread_cleanup_pop(1);
> > +
> > + if (rc == FOREIGN_CLAIMED)
> > + condlog(3, "%s: %s: added map %s", __func__, THIS,
> > + udev_device_get_sysname(ud));
> > + else if (rc != FOREIGN_OK)
> > + condlog(1, "%s: %s: retcode %d adding %s",
> > + __func__, THIS, rc,
> > udev_device_get_sysname(ud));
> > +
> > + return rc;
> > +}
> > +
> > +int change(struct context *ctx, struct udev_device *ud)
> > +{
> > + condlog(5, "%s called for \"%s\"", __func__, THIS);
> > + return FOREIGN_IGNORED;
> > +}
> > +
> > +static int _delete_map(struct context *ctx, struct udev_device
> > *ud)
> > +{
> > + int k;
> > + struct nvme_map *map;
> > + dev_t devt = udev_device_get_devnum(ud);
> > +
> > + map = _find_nvme_map_by_devt(ctx, devt);
> > + if (map ==NULL)
> > + return FOREIGN_IGNORED;
> > +
> > + k = find_slot(ctx->mpvec, map);
> > + if (k == -1)
> > + return FOREIGN_ERR;
> > + else
> > + vector_del_slot(ctx->mpvec, k);
> > +
> > + cleanup_nvme_map(map);
> > +
> > + return FOREIGN_OK;
> > +}
> > +
> > +int delete(struct context *ctx, struct udev_device *ud)
> > +{
> > + int rc;
> > +
> > + condlog(5, "%s called for \"%s\"", __func__, THIS);
> > +
> > + if (ud == NULL)
> > + return FOREIGN_ERR;
> > +
> > + lock(ctx);
> > + pthread_cleanup_push(unlock, ctx);
> > + rc = _delete_map(ctx, ud);
> > + pthread_cleanup_pop(1);
> > +
> > + if (rc == FOREIGN_OK)
> > + condlog(3, "%s: %s: map %s deleted", __func__,
> > THIS,
> > + udev_device_get_sysname(ud));
> > + else if (rc != FOREIGN_IGNORED)
> > + condlog(1, "%s: %s: retcode %d deleting map %s",
> > __func__,
> > + THIS, rc, udev_device_get_sysname(ud));
> > +
> > + return rc;
> > +}
> > +
> > +void check(struct context *ctx)
> > +{
> > + condlog(5, "%s called for \"%s\"", __func__, THIS);
> > + return;
> > +}
> > +
> > +/*
> > + * It's safe to pass our internal pointer, this is only used under
> > the lock.
> > + */
> > +const struct _vector *get_multipaths(const struct context *ctx)
> > +{
> > + condlog(5, "%s called for \"%s\"", __func__, THIS);
> > + return ctx->mpvec;
> > +}
> > +
> > +void release_multipaths(const struct context *ctx, const struct
> > _vector *mpvec)
> > +{
> > + condlog(5, "%s called for \"%s\"", __func__, THIS);
> > + /* NOP */
> > +}
> > +
> > +/*
> > + * It's safe to pass our internal pointer, this is only used under
> > the lock.
> > + */
> > +const struct _vector * get_paths(const struct context *ctx)
> > +{
> > + condlog(5, "%s called for \"%s\"", __func__, THIS);
> > + return NULL;
> > +}
> > +
> > +void release_paths(const struct context *ctx, const struct _vector
> > *mpvec)
> > +{
> > + condlog(5, "%s called for \"%s\"", __func__, THIS);
> > + /* NOP */
> > +}
> > +
> > +/* compile-time check whether all methods are present and
> > correctly typed */
> > +#define _METHOD_INIT(x) .x = x
> > +static struct foreign __methods __attribute__((unused)) = {
> > + _METHOD_INIT(init),
> > + _METHOD_INIT(cleanup),
> > + _METHOD_INIT(change),
> > + _METHOD_INIT(delete),
> > + _METHOD_INIT(delete_all),
> > + _METHOD_INIT(check),
> > + _METHOD_INIT(lock),
> > + _METHOD_INIT(unlock),
> > + _METHOD_INIT(get_multipaths),
> > + _METHOD_INIT(release_multipaths),
> > + _METHOD_INIT(get_paths),
> > + _METHOD_INIT(release_paths),
> > +};
> > --
> > 2.16.1
>
>
--
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
More information about the dm-devel
mailing list