> On 16 Apr 2019, at 12.16, Igor Konopko <igor.j.konopko@xxxxxxxxx> wrote:
>
> Currently all the target instances are removed under global nvm_lock.
> This was needed to ensure that nvm_dev struct will not be freed by
> hot unplug event during target removal. However, current implementation
> has some drawbacks, since the same lock is used when new nvme subsystem
> is registered, so we can have a situation, that due to long process of
> target removal on drive A, registration (and listing in OS) of the
> drive B will take a lot of time, since it will wait for that lock.
>
> Now when we have kref which ensures that nvm_dev will not be freed in
> the meantime, we can easily get rid of this lock for a time when we are
> removing nvm targets.
>
> Signed-off-by: Igor Konopko <igor.j.konopko@xxxxxxxxx>
> ---
> drivers/lightnvm/core.c | 34 ++++++++++++++++------------------
> 1 file changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index 0e9f7996..0df7454 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -483,7 +483,6 @@ static void __nvm_remove_target(struct nvm_target *t, bool graceful)
>
> /**
> * nvm_remove_tgt - Removes a target from the media manager
> - * @dev: device
> * @remove: ioctl structure with target name to remove.
> *
> * Returns:
> @@ -491,18 +490,27 @@ static void __nvm_remove_target(struct nvm_target *t, bool graceful)
> * 1: on not found
> * <0: on error
> */
> -static int nvm_remove_tgt(struct nvm_dev *dev, struct nvm_ioctl_remove *remove)
> +static int nvm_remove_tgt(struct nvm_ioctl_remove *remove)
> {
> struct nvm_target *t;
> + struct nvm_dev *dev;
>
> - mutex_lock(&dev->mlock);
> - t = nvm_find_target(dev, remove->tgtname);
> - if (!t) {
> + down_read(&nvm_lock);
> + list_for_each_entry(dev, &nvm_devices, devices) {
> + mutex_lock(&dev->mlock);
> + t = nvm_find_target(dev, remove->tgtname);
> + if (t) {
> + mutex_unlock(&dev->mlock);
> + break;
> + }
> mutex_unlock(&dev->mlock);
> - return 1;
> }
> + up_read(&nvm_lock);
> +
> + if (!t)
> + return 1;
> +
> __nvm_remove_target(t, true);
> - mutex_unlock(&dev->mlock);
> kref_put(&dev->ref, nvm_free);
>
> return 0;
> @@ -1348,8 +1356,6 @@ static long nvm_ioctl_dev_create(struct file *file, void __user *arg)
> static long nvm_ioctl_dev_remove(struct file *file, void __user *arg)
> {
> struct nvm_ioctl_remove remove;
> - struct nvm_dev *dev;
> - int ret = 0;
>
> if (copy_from_user(&remove, arg, sizeof(struct nvm_ioctl_remove)))
> return -EFAULT;
> @@ -1361,15 +1367,7 @@ static long nvm_ioctl_dev_remove(struct file *file, void __user *arg)
> return -EINVAL;
> }
>
> - down_read(&nvm_lock);
> - list_for_each_entry(dev, &nvm_devices, devices) {
> - ret = nvm_remove_tgt(dev, &remove);
> - if (!ret)
> - break;
> - }
> - up_read(&nvm_lock);
> -
> - return ret;
> + return nvm_remove_tgt(&remove);
> }
>
> /* kept for compatibility reasons */
> --
> 2.9.5
Looks good to me
Reviewed-by: Javier González <javier@xxxxxxxxxxx>
Attachment:
signature.asc
Description: Message signed with OpenPGP
