| Message ID | 20190204211751.19001-2-jgg@ziepe.ca (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Rework device, client and client data locking | expand |
On 04-Feb-19 23:17, Jason Gunthorpe wrote: > From: Jason Gunthorpe <jgg@mellanox.com> > > Since another rename could be running in parallel it is safer to check > that the name is not changing inside the lock, where we already know the > device name will not change. > > Fixes: d21943dd19b5 ("RDMA/core: Implement IB device rename function") > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > Reviewed-by: Parav Pandit <parav@mellanox.com> > --- > drivers/infiniband/core/device.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > index 55221990d946b2..f9f33e842c16b6 100644 > --- a/drivers/infiniband/core/device.c > +++ b/drivers/infiniband/core/device.c > @@ -189,12 +189,14 @@ static struct ib_device *__ib_device_get_by_name(const char *name) > > int ib_device_rename(struct ib_device *ibdev, const char *name) > { > - int ret = 0; > - > - if (!strcmp(name, dev_name(&ibdev->dev))) > - return ret; > + int ret; > > mutex_lock(&device_mutex); > + if (!strcmp(name, dev_name(&ibdev->dev))) { > + ret = -EEXIST; Why did this change to an error? Probably worth noting that rename to the same name no longer returns success in the commit message. > + goto out; > + } > + > if (__ib_device_get_by_name(name)) { > ret = -EEXIST; > goto out; >
On Tue, Feb 05, 2019 at 04:14:28PM +0200, Gal Pressman wrote: > On 04-Feb-19 23:17, Jason Gunthorpe wrote: > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > Since another rename could be running in parallel it is safer to check > > that the name is not changing inside the lock, where we already know the > > device name will not change. > > > > Fixes: d21943dd19b5 ("RDMA/core: Implement IB device rename function") > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > Reviewed-by: Parav Pandit <parav@mellanox.com> > > drivers/infiniband/core/device.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > > index 55221990d946b2..f9f33e842c16b6 100644 > > +++ b/drivers/infiniband/core/device.c > > @@ -189,12 +189,14 @@ static struct ib_device *__ib_device_get_by_name(const char *name) > > > > int ib_device_rename(struct ib_device *ibdev, const char *name) > > { > > - int ret = 0; > > - > > - if (!strcmp(name, dev_name(&ibdev->dev))) > > - return ret; > > + int ret; > > > > mutex_lock(&device_mutex); > > + if (!strcmp(name, dev_name(&ibdev->dev))) { > > + ret = -EEXIST; > > Why did this change to an error? Hmm.. Probably shouldn't change it - even though I think this version is more appropriate. Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Tuesday, February 5, 2019 10:05 AM > To: Gal Pressman <galpress@amazon.com> > Cc: linux-rdma@vger.kernel.org; Parav Pandit <parav@mellanox.com> > Subject: Re: [PATCH 1/8] RDMA/device: Check that the rename is nop under > the lock > > On Tue, Feb 05, 2019 at 04:14:28PM +0200, Gal Pressman wrote: > > On 04-Feb-19 23:17, Jason Gunthorpe wrote: > > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > > > Since another rename could be running in parallel it is safer to > > > check that the name is not changing inside the lock, where we > > > already know the device name will not change. > > > > > > Fixes: d21943dd19b5 ("RDMA/core: Implement IB device rename > > > function") > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > > Reviewed-by: Parav Pandit <parav@mellanox.com> > > > drivers/infiniband/core/device.c | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/infiniband/core/device.c > > > b/drivers/infiniband/core/device.c > > > index 55221990d946b2..f9f33e842c16b6 100644 > > > +++ b/drivers/infiniband/core/device.c > > > @@ -189,12 +189,14 @@ static struct ib_device > > > *__ib_device_get_by_name(const char *name) > > > > > > int ib_device_rename(struct ib_device *ibdev, const char *name) { > > > - int ret = 0; > > > - > > > - if (!strcmp(name, dev_name(&ibdev->dev))) > > > - return ret; > > > + int ret; > > > > > > mutex_lock(&device_mutex); > > > + if (!strcmp(name, dev_name(&ibdev->dev))) { > > > + ret = -EEXIST; > > > > Why did this change to an error? > > Hmm.. Probably shouldn't change it - even though I think this version is > more appropriate. > I think we should return 0 for same name. > Jason
On Tue, Feb 05, 2019 at 04:24:09PM +0000, Parav Pandit wrote: > > > > -----Original Message----- > > From: Jason Gunthorpe <jgg@ziepe.ca> > > Sent: Tuesday, February 5, 2019 10:05 AM > > To: Gal Pressman <galpress@amazon.com> > > Cc: linux-rdma@vger.kernel.org; Parav Pandit <parav@mellanox.com> > > Subject: Re: [PATCH 1/8] RDMA/device: Check that the rename is nop under > > the lock > > > > On Tue, Feb 05, 2019 at 04:14:28PM +0200, Gal Pressman wrote: > > > On 04-Feb-19 23:17, Jason Gunthorpe wrote: > > > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > > > > > Since another rename could be running in parallel it is safer to > > > > check that the name is not changing inside the lock, where we > > > > already know the device name will not change. > > > > > > > > Fixes: d21943dd19b5 ("RDMA/core: Implement IB device rename > > > > function") > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > > > Reviewed-by: Parav Pandit <parav@mellanox.com> > > > > drivers/infiniband/core/device.c | 10 ++++++---- > > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/core/device.c > > > > b/drivers/infiniband/core/device.c > > > > index 55221990d946b2..f9f33e842c16b6 100644 > > > > +++ b/drivers/infiniband/core/device.c > > > > @@ -189,12 +189,14 @@ static struct ib_device > > > > *__ib_device_get_by_name(const char *name) > > > > > > > > int ib_device_rename(struct ib_device *ibdev, const char *name) { > > > > - int ret = 0; > > > > - > > > > - if (!strcmp(name, dev_name(&ibdev->dev))) > > > > - return ret; > > > > + int ret; > > > > > > > > mutex_lock(&device_mutex); > > > > + if (!strcmp(name, dev_name(&ibdev->dev))) { > > > > + ret = -EEXIST; > > > > > > Why did this change to an error? > > > > Hmm.. Probably shouldn't change it - even though I think this version is > > more appropriate. > > > I think we should return 0 for same name. Agree, I returned 0 because was under expectation that we can have multiple parallel calls from user space to do rename and it is not an error to try to rename to already existing name. Thanks > > > Jason
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index 55221990d946b2..f9f33e842c16b6 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -189,12 +189,14 @@ static struct ib_device *__ib_device_get_by_name(const char *name) int ib_device_rename(struct ib_device *ibdev, const char *name) { - int ret = 0; - - if (!strcmp(name, dev_name(&ibdev->dev))) - return ret; + int ret; mutex_lock(&device_mutex); + if (!strcmp(name, dev_name(&ibdev->dev))) { + ret = -EEXIST; + goto out; + } + if (__ib_device_get_by_name(name)) { ret = -EEXIST; goto out;