[PATCH 0/4] rbd: locking, snapshot registration, snap_by_name()

[PATCH 0/4] rbd: locking, snapshot registration, snap_by_name()

[Alex Elder]

The patches in this fourth series is not very well related to
each other.

The first two make the bulk of the initialization of an rbd_dev
occur while holding its semaphore.  It's done in two steps to
make review easier.

The third deletes some code that some analysis shows is not
necessary.

The fourth one is significant for version 2 images.  It changes
rbd_snap_by_name() so it uses the rbd_dev->snaps list in looking
up a snapshot id by name rather than using the snapshot context.
For version 2 images, having a snapshot context doesn't imply
the names for the snapshot ids in that context are readily available.
However all snapshots in the snaps list will already have a cached
copy of snapshot names (for both format 1 and 2) so this allows
this single lookup function to work efficiently for both formats.

It is available as branch "wip-rbd-review-4" on the ceph-client git
repository, and is based on the branch "wip-rbd-review-3".

    https://github.com/ceph/ceph-client/tree/wip-rbd-review-4

					-Alex

[PATCH 1/4]  f3b3dfe rbd: move locking out of rbd_header_set_snap()
[PATCH 2/4]  aecdaf1 rbd: expand lock protection in rbd_add()
[PATCH 3/4]  9278d3a rbd: don't register snapshots in bus_add_dev()
[PATCH 4/4]  dcc54d7 rbd: use snaps list in rbd_snap_by_name()

[PATCH 1/4] rbd: move locking out of rbd_header_set_snap()

[Alex Elder]

Move the calls to get the header semaphore out of
rbd_header_set_snap() and into its caller.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 634a16c..214c937 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -647,8 +647,6 @@ static int rbd_header_set_snap(struct rbd_device
*rbd_dev, char *snap_name)
 {
 	int ret;

-	down_write(&rbd_dev->header_rwsem);
-
 	if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME,
 		    sizeof (RBD_SNAP_HEAD_NAME))) {
 		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
@@ -666,7 +664,6 @@ static int rbd_header_set_snap(struct rbd_device
*rbd_dev, char *snap_name)

 	ret = 0;
 done:
-	up_write(&rbd_dev->header_rwsem);
 	return ret;
 }

@@ -2608,7 +2605,9 @@ static ssize_t rbd_add(struct bus_type *bus,
 	if (rc)
 		goto err_out_bus;

+	down_write(&rbd_dev->header_rwsem);
 	rc = rbd_header_set_snap(rbd_dev, snap_name);
+	up_write(&rbd_dev->header_rwsem);
 	if (rc)
 		goto err_out_bus;

-- 
1.7.9.5

[PATCH 2/4] rbd: expand lock protection in rbd_add()

[Alex Elder]

Expand the region of code in rbd_add() protected by the header
semaphore to include the complete initialization sequence.  It may
not be strictly necessary, but it doesn't hurt.  And with the
upcoming changes to the order of steps here this offers easy
protection.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 214c937..6af09f1 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2553,6 +2553,8 @@ static ssize_t rbd_add(struct bus_type *bus,
 	INIT_LIST_HEAD(&rbd_dev->snaps);
 	init_rwsem(&rbd_dev->header_rwsem);

+	down_write(&rbd_dev->header_rwsem);
+
 	/* generate unique id: find highest unique id, add one */
 	rbd_dev_id_get(rbd_dev);

@@ -2598,18 +2600,17 @@ static ssize_t rbd_add(struct bus_type *bus,
 	/* contact OSD, request size info about the object being mapped */
 	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
 	if (rc)
-		goto err_out_bus;
+		goto err_out_unlock;

-	/* no need to lock here, as rbd_dev is not registered yet */
 	rc = rbd_dev_snap_devs_update(rbd_dev);
 	if (rc)
-		goto err_out_bus;
+		goto err_out_unlock;

-	down_write(&rbd_dev->header_rwsem);
 	rc = rbd_header_set_snap(rbd_dev, snap_name);
-	up_write(&rbd_dev->header_rwsem);
 	if (rc)
-		goto err_out_bus;
+		goto err_out_unlock;
+
+	up_write(&rbd_dev->header_rwsem);

 	/* Set up the blkdev mapping. */

@@ -2630,6 +2631,8 @@ static ssize_t rbd_add(struct bus_type *bus,

 	return count;

+err_out_unlock:
+	up_write(&rbd_dev->header_rwsem);
 err_out_bus:
 	/* this will also clean up rest of rbd_dev stuff */

@@ -2649,6 +2652,7 @@ err_put_id:
 		kfree(rbd_dev->pool_name);
 	}
 	rbd_dev_id_put(rbd_dev);
+	up_write(&rbd_dev->header_rwsem);
 err_nomem:
 	kfree(rbd_dev);
 	kfree(options);
-- 
1.7.9.5

[PATCH 3/4] rbd: don't register snapshots in bus_add_dev()

[Alex Elder]

When rbd_bus_add_dev() is called (one spot--in rbd_add()), the rbd
image header has not even been read yet.  This means that the list
of snapshots will be empty at the time of the call.  As a result,
there is no need for the code that calls rbd_register_snap_dev()
for each entry in that list--so get rid of it.

Once the header has been read (just after returning), a call will
be made to rbd_dev_snap_devs_update(), which will then find every
snapshot in the context to be new and will therefore call
rbd_register_snap_dev() via __rbd_add_snap_dev() accomplishing
the same thing.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 6af09f1..e922989 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2244,29 +2244,21 @@ static int rbd_dev_snap_devs_update(struct
rbd_device *rbd_dev)

 static int rbd_bus_add_dev(struct rbd_device *rbd_dev)
 {
-	int ret;
 	struct device *dev;
-	struct rbd_snap *snap;
+	int ret;

 	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-	dev = &rbd_dev->dev;

+	dev = &rbd_dev->dev;
 	dev->bus = &rbd_bus_type;
 	dev->type = &rbd_device_type;
 	dev->parent = &rbd_root_dev;
 	dev->release = rbd_dev_release;
 	dev_set_name(dev, "%d", rbd_dev->dev_id);
 	ret = device_register(dev);
-	if (ret < 0)
-		goto out;

-	list_for_each_entry(snap, &rbd_dev->snaps, node) {
-		ret = rbd_register_snap_dev(snap, &rbd_dev->dev);
-		if (ret < 0)
-			break;
-	}
-out:
 	mutex_unlock(&ctl_mutex);
+
 	return ret;
 }

-- 
1.7.9.5

[PATCH 4/4] rbd: use snaps list in rbd_snap_by_name()

[Alex Elder]

An rbd_dev structure maintains a list of current snapshots that have
already been fully initialized.  The entries on the list have type
struct rbd_snap, and each entry contains a copy of information
that's found in the rbd_dev's snapshot context and header.

The only caller of snap_by_name() is rbd_header_set_snap().  In that
call site any positive return value (the index in the snapshot
array) is ignored.

rbd_header_set_snap() also has only one caller--rbd_add()--and that
call is made after a call to rbd_dev_snap_devs_update().  Because
the rbd_snap structures are initialized in that function, the
current snapshot list can be used instead of the snapshot context to
look up a snapshot's information by name.

Change snap_by_name() so it uses the snapshot list rather than the
rbd_dev's snapshot context in looking up snapshot information.
Return 0 if it's found rather than the snapshot id.  To do this,
change rbd_snap_by_name() to take an rbd_dev rather than
rbd_image_header structure pointer as its first argument.

No caller ever passes a null pointer to snap_by_name() for the
snapshot id or the size, so just assign using those pointers
unconditionally.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index e922989..4dff92f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -623,23 +623,18 @@ out_err:

 static int snap_by_name(struct rbd_device *rbd_dev, const char *snap_name)
 {
-	int i;
-	struct rbd_image_header *header = &rbd_dev->header;
-	char *p = header->snap_names;
-
-	rbd_assert(header->snapc != NULL);
-	for (i = 0; i < header->snapc->num_snaps; i++) {
-		if (!strcmp(snap_name, p)) {

-			/* Found it.  Pass back its id and/or size */
+	struct rbd_snap *snap;

-			rbd_dev->mapping.snap_id = header->snapc->snaps[i];
-			rbd_dev->mapping.size = header->snap_sizes[i];
+	list_for_each_entry(snap, &rbd_dev->snaps, node) {
+		if (!strcmp(snap_name, snap->name)) {
+			rbd_dev->mapping.snap_id = snap->id;
+			rbd_dev->mapping.size = snap->size;

-			return i;
+			return 0;
 		}
-		p += strlen(p) + 1;	/* Skip ahead to the next name */
 	}
+
 	return -ENOENT;
 }

-- 
1.7.9.5

Re: [PATCH 1/4] rbd: move locking out of rbd_header_set_snap()

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 09/07/2012 07:50 AM, Alex Elder wrote:
> Move the calls to get the header semaphore out of
> rbd_header_set_snap() and into its caller.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 634a16c..214c937 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -647,8 +647,6 @@ static int rbd_header_set_snap(struct rbd_device
> *rbd_dev, char *snap_name)
>   {
>   	int ret;
>
> -	down_write(&rbd_dev->header_rwsem);
> -
>   	if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME,
>   		    sizeof (RBD_SNAP_HEAD_NAME))) {
>   		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
> @@ -666,7 +664,6 @@ static int rbd_header_set_snap(struct rbd_device
> *rbd_dev, char *snap_name)
>
>   	ret = 0;
>   done:
> -	up_write(&rbd_dev->header_rwsem);
>   	return ret;
>   }
>
> @@ -2608,7 +2605,9 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	if (rc)
>   		goto err_out_bus;
>
> +	down_write(&rbd_dev->header_rwsem);
>   	rc = rbd_header_set_snap(rbd_dev, snap_name);
> +	up_write(&rbd_dev->header_rwsem);
>   	if (rc)
>   		goto err_out_bus;
>

Re: [PATCH 2/4] rbd: expand lock protection in rbd_add()

[Josh Durgin]

Looking closer, I don't think we need to protect this section at all.
The device isn't initialized yet, so nothing else can access the
rbd_dev->header. It'd be good to have a comment to that effect.

Josh

On 09/07/2012 07:50 AM, Alex Elder wrote:
> Expand the region of code in rbd_add() protected by the header
> semaphore to include the complete initialization sequence.  It may
> not be strictly necessary, but it doesn't hurt.  And with the
> upcoming changes to the order of steps here this offers easy
> protection.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 214c937..6af09f1 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2553,6 +2553,8 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	INIT_LIST_HEAD(&rbd_dev->snaps);
>   	init_rwsem(&rbd_dev->header_rwsem);
>
> +	down_write(&rbd_dev->header_rwsem);
> +
>   	/* generate unique id: find highest unique id, add one */
>   	rbd_dev_id_get(rbd_dev);
>
> @@ -2598,18 +2600,17 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	/* contact OSD, request size info about the object being mapped */
>   	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
>   	if (rc)
> -		goto err_out_bus;
> +		goto err_out_unlock;
>
> -	/* no need to lock here, as rbd_dev is not registered yet */
>   	rc = rbd_dev_snap_devs_update(rbd_dev);
>   	if (rc)
> -		goto err_out_bus;
> +		goto err_out_unlock;
>
> -	down_write(&rbd_dev->header_rwsem);
>   	rc = rbd_header_set_snap(rbd_dev, snap_name);
> -	up_write(&rbd_dev->header_rwsem);
>   	if (rc)
> -		goto err_out_bus;
> +		goto err_out_unlock;
> +
> +	up_write(&rbd_dev->header_rwsem);
>
>   	/* Set up the blkdev mapping. */
>
> @@ -2630,6 +2631,8 @@ static ssize_t rbd_add(struct bus_type *bus,
>
>   	return count;
>
> +err_out_unlock:
> +	up_write(&rbd_dev->header_rwsem);
>   err_out_bus:
>   	/* this will also clean up rest of rbd_dev stuff */
>
> @@ -2649,6 +2652,7 @@ err_put_id:
>   		kfree(rbd_dev->pool_name);
>   	}
>   	rbd_dev_id_put(rbd_dev);
> +	up_write(&rbd_dev->header_rwsem);
>   err_nomem:
>   	kfree(rbd_dev);
>   	kfree(options);
>

Re: [PATCH 3/4] rbd: don't register snapshots in bus_add_dev()

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 09/07/2012 07:50 AM, Alex Elder wrote:
> When rbd_bus_add_dev() is called (one spot--in rbd_add()), the rbd
> image header has not even been read yet.  This means that the list
> of snapshots will be empty at the time of the call.  As a result,
> there is no need for the code that calls rbd_register_snap_dev()
> for each entry in that list--so get rid of it.
>
> Once the header has been read (just after returning), a call will
> be made to rbd_dev_snap_devs_update(), which will then find every
> snapshot in the context to be new and will therefore call
> rbd_register_snap_dev() via __rbd_add_snap_dev() accomplishing
> the same thing.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   14 +++-----------
>   1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 6af09f1..e922989 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2244,29 +2244,21 @@ static int rbd_dev_snap_devs_update(struct
> rbd_device *rbd_dev)
>
>   static int rbd_bus_add_dev(struct rbd_device *rbd_dev)
>   {
> -	int ret;
>   	struct device *dev;
> -	struct rbd_snap *snap;
> +	int ret;
>
>   	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> -	dev = &rbd_dev->dev;
>
> +	dev = &rbd_dev->dev;
>   	dev->bus = &rbd_bus_type;
>   	dev->type = &rbd_device_type;
>   	dev->parent = &rbd_root_dev;
>   	dev->release = rbd_dev_release;
>   	dev_set_name(dev, "%d", rbd_dev->dev_id);
>   	ret = device_register(dev);
> -	if (ret < 0)
> -		goto out;
>
> -	list_for_each_entry(snap, &rbd_dev->snaps, node) {
> -		ret = rbd_register_snap_dev(snap, &rbd_dev->dev);
> -		if (ret < 0)
> -			break;
> -	}
> -out:
>   	mutex_unlock(&ctl_mutex);
> +
>   	return ret;
>   }
>

Re: [PATCH 4/4] rbd: use snaps list in rbd_snap_by_name()

[Josh Durgin]

On 09/07/2012 07:50 AM, Alex Elder wrote:
> An rbd_dev structure maintains a list of current snapshots that have
> already been fully initialized.  The entries on the list have type
> struct rbd_snap, and each entry contains a copy of information
> that's found in the rbd_dev's snapshot context and header.
>
> The only caller of snap_by_name() is rbd_header_set_snap().  In that
> call site any positive return value (the index in the snapshot
> array) is ignored.
>
> rbd_header_set_snap() also has only one caller--rbd_add()--and that
> call is made after a call to rbd_dev_snap_devs_update().  Because
> the rbd_snap structures are initialized in that function, the
> current snapshot list can be used instead of the snapshot context to
> look up a snapshot's information by name.
>
> Change snap_by_name() so it uses the snapshot list rather than the
> rbd_dev's snapshot context in looking up snapshot information.
> Return 0 if it's found rather than the snapshot id.  To do this,
> change rbd_snap_by_name() to take an rbd_dev rather than
> rbd_image_header structure pointer as its first argument.

This argument change was an earlier commit now.

> No caller ever passes a null pointer to snap_by_name() for the
> snapshot id or the size, so just assign using those pointers
> unconditionally.

No caller passes id or size now either.

With the commit message fixed:

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   19 +++++++------------
>   1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index e922989..4dff92f 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -623,23 +623,18 @@ out_err:
>
>   static int snap_by_name(struct rbd_device *rbd_dev, const char *snap_name)
>   {
> -	int i;
> -	struct rbd_image_header *header = &rbd_dev->header;
> -	char *p = header->snap_names;
> -
> -	rbd_assert(header->snapc != NULL);
> -	for (i = 0; i < header->snapc->num_snaps; i++) {
> -		if (!strcmp(snap_name, p)) {
>
> -			/* Found it.  Pass back its id and/or size */
> +	struct rbd_snap *snap;
>
> -			rbd_dev->mapping.snap_id = header->snapc->snaps[i];
> -			rbd_dev->mapping.size = header->snap_sizes[i];
> +	list_for_each_entry(snap, &rbd_dev->snaps, node) {
> +		if (!strcmp(snap_name, snap->name)) {
> +			rbd_dev->mapping.snap_id = snap->id;
> +			rbd_dev->mapping.size = snap->size;
>
> -			return i;
> +			return 0;
>   		}
> -		p += strlen(p) + 1;	/* Skip ahead to the next name */
>   	}
> +
>   	return -ENOENT;
>   }
>

Re: [PATCH 2/4] rbd: expand lock protection in rbd_add()

[Alex Elder]

On 09/10/2012 05:05 PM, Josh Durgin wrote:
> Looking closer, I don't think we need to protect this section at all.
> The device isn't initialized yet, so nothing else can access the
> rbd_dev->header. It'd be good to have a comment to that effect.
> 
> Josh

For the most part that's true.

However as soon as it's possible to refresh the header, the
lock should be held.  And in the code as of this point that
happens as soon as rbd_bus_add_dev() gets called.  In that
respect this change fixes a bug, because the header refresh
could have been initiated before the header had even been
read yet.

Upcoming patches rearrange the order of this stuff quite a
bit though, which was why I just grabbed the lock immediately
and held onto it until everything is set up, for simplicity.

After all of these are in place I can reduce the range over
which the lock is held during initialization.  But as I
said in the commit commit, it doesn't really hurt to hold
it.  I will add a comment in the code to that effect before
I commit.

					-Alex

> 
> On 09/07/2012 07:50 AM, Alex Elder wrote:
>> Expand the region of code in rbd_add() protected by the header
>> semaphore to include the complete initialization sequence.  It may
>> not be strictly necessary, but it doesn't hurt.  And with the
>> upcoming changes to the order of steps here this offers easy
>> protection.
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   16 ++++++++++------
>>   1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 214c937..6af09f1 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -2553,6 +2553,8 @@ static ssize_t rbd_add(struct bus_type *bus,
>>       INIT_LIST_HEAD(&rbd_dev->snaps);
>>       init_rwsem(&rbd_dev->header_rwsem);
>>
>> +    down_write(&rbd_dev->header_rwsem);
>> +
>>       /* generate unique id: find highest unique id, add one */
>>       rbd_dev_id_get(rbd_dev);
>>
>> @@ -2598,18 +2600,17 @@ static ssize_t rbd_add(struct bus_type *bus,
>>       /* contact OSD, request size info about the object being mapped */
>>       rc = rbd_read_header(rbd_dev, &rbd_dev->header);
>>       if (rc)
>> -        goto err_out_bus;
>> +        goto err_out_unlock;
>>
>> -    /* no need to lock here, as rbd_dev is not registered yet */
>>       rc = rbd_dev_snap_devs_update(rbd_dev);
>>       if (rc)
>> -        goto err_out_bus;
>> +        goto err_out_unlock;
>>
>> -    down_write(&rbd_dev->header_rwsem);
>>       rc = rbd_header_set_snap(rbd_dev, snap_name);
>> -    up_write(&rbd_dev->header_rwsem);
>>       if (rc)
>> -        goto err_out_bus;
>> +        goto err_out_unlock;
>> +
>> +    up_write(&rbd_dev->header_rwsem);
>>
>>       /* Set up the blkdev mapping. */
>>
>> @@ -2630,6 +2631,8 @@ static ssize_t rbd_add(struct bus_type *bus,
>>
>>       return count;
>>
>> +err_out_unlock:
>> +    up_write(&rbd_dev->header_rwsem);
>>   err_out_bus:
>>       /* this will also clean up rest of rbd_dev stuff */
>>
>> @@ -2649,6 +2652,7 @@ err_put_id:
>>           kfree(rbd_dev->pool_name);
>>       }
>>       rbd_dev_id_put(rbd_dev);
>> +    up_write(&rbd_dev->header_rwsem);
>>   err_nomem:
>>       kfree(rbd_dev);
>>       kfree(options);
>>
> 
> 
> 

Re: [PATCH 4/4] rbd: use snaps list in rbd_snap_by_name()

[Alex Elder]

On 09/10/2012 05:32 PM, Josh Durgin wrote:
> On 09/07/2012 07:50 AM, Alex Elder wrote:
>> An rbd_dev structure maintains a list of current snapshots that have
>> already been fully initialized.  The entries on the list have type
>> struct rbd_snap, and each entry contains a copy of information
>> that's found in the rbd_dev's snapshot context and header.
>>
>> The only caller of snap_by_name() is rbd_header_set_snap().  In that
>> call site any positive return value (the index in the snapshot
>> array) is ignored.
>>
>> rbd_header_set_snap() also has only one caller--rbd_add()--and that
>> call is made after a call to rbd_dev_snap_devs_update().  Because
>> the rbd_snap structures are initialized in that function, the
>> current snapshot list can be used instead of the snapshot context to
>> look up a snapshot's information by name.
>>
>> Change snap_by_name() so it uses the snapshot list rather than the
>> rbd_dev's snapshot context in looking up snapshot information.
>> Return 0 if it's found rather than the snapshot id.  To do this,
>> change rbd_snap_by_name() to take an rbd_dev rather than
>> rbd_image_header structure pointer as its first argument.
> 
> This argument change was an earlier commit now.

Whoops.  I need to pay more attention to the descriptions before
I post for review I guess.  Apparently I decided to pull that
change into its own patch somewhere along the way...

>> No caller ever passes a null pointer to snap_by_name() for the
>> snapshot id or the size, so just assign using those pointers
>> unconditionally.
> 
> No caller passes id or size now either.

Here again, moved that particular change elsewhere.  Good eye.

> With the commit message fixed:

Will do.

> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
> 
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   19 +++++++------------
>>   1 file changed, 7 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index e922989..4dff92f 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -623,23 +623,18 @@ out_err:
>>
>>   static int snap_by_name(struct rbd_device *rbd_dev, const char
>> *snap_name)
>>   {
>> -    int i;
>> -    struct rbd_image_header *header = &rbd_dev->header;
>> -    char *p = header->snap_names;
>> -
>> -    rbd_assert(header->snapc != NULL);
>> -    for (i = 0; i < header->snapc->num_snaps; i++) {
>> -        if (!strcmp(snap_name, p)) {
>>
>> -            /* Found it.  Pass back its id and/or size */
>> +    struct rbd_snap *snap;
>>
>> -            rbd_dev->mapping.snap_id = header->snapc->snaps[i];
>> -            rbd_dev->mapping.size = header->snap_sizes[i];
>> +    list_for_each_entry(snap, &rbd_dev->snaps, node) {
>> +        if (!strcmp(snap_name, snap->name)) {
>> +            rbd_dev->mapping.snap_id = snap->id;
>> +            rbd_dev->mapping.size = snap->size;
>>
>> -            return i;
>> +            return 0;
>>           }
>> -        p += strlen(p) + 1;    /* Skip ahead to the next name */
>>       }
>> +
>>       return -ENOENT;
>>   }
>>
> 
> 
> 

Re: [PATCH 2/4] rbd: expand lock protection in rbd_add()

[Josh Durgin]

On 09/11/2012 06:52 AM, Alex Elder wrote:
> On 09/10/2012 05:05 PM, Josh Durgin wrote:
>> Looking closer, I don't think we need to protect this section at all.
>> The device isn't initialized yet, so nothing else can access the
>> rbd_dev->header. It'd be good to have a comment to that effect.
>>
>> Josh
>
> For the most part that's true.
>
> However as soon as it's possible to refresh the header, the
> lock should be held.  And in the code as of this point that
> happens as soon as rbd_bus_add_dev() gets called.  In that
> respect this change fixes a bug, because the header refresh
> could have been initiated before the header had even been
> read yet.
>
> Upcoming patches rearrange the order of this stuff quite a
> bit though, which was why I just grabbed the lock immediately
> and held onto it until everything is set up, for simplicity.
>
> After all of these are in place I can reduce the range over
> which the lock is held during initialization.  But as I
> said in the commit commit, it doesn't really hurt to hold
> it.  I will add a comment in the code to that effect before
> I commit.
>
> 					-Alex

OK, I see what you mean now. I'd forgotten about the possible manual
refresh via sysfs.

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

>>
>> On 09/07/2012 07:50 AM, Alex Elder wrote:
>>> Expand the region of code in rbd_add() protected by the header
>>> semaphore to include the complete initialization sequence.  It may
>>> not be strictly necessary, but it doesn't hurt.  And with the
>>> upcoming changes to the order of steps here this offers easy
>>> protection.
>>>
>>> Signed-off-by: Alex Elder <elder@inktank.com>
>>> ---
>>>    drivers/block/rbd.c |   16 ++++++++++------
>>>    1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>>> index 214c937..6af09f1 100644
>>> --- a/drivers/block/rbd.c
>>> +++ b/drivers/block/rbd.c
>>> @@ -2553,6 +2553,8 @@ static ssize_t rbd_add(struct bus_type *bus,
>>>        INIT_LIST_HEAD(&rbd_dev->snaps);
>>>        init_rwsem(&rbd_dev->header_rwsem);
>>>
>>> +    down_write(&rbd_dev->header_rwsem);
>>> +
>>>        /* generate unique id: find highest unique id, add one */
>>>        rbd_dev_id_get(rbd_dev);
>>>
>>> @@ -2598,18 +2600,17 @@ static ssize_t rbd_add(struct bus_type *bus,
>>>        /* contact OSD, request size info about the object being mapped */
>>>        rc = rbd_read_header(rbd_dev, &rbd_dev->header);
>>>        if (rc)
>>> -        goto err_out_bus;
>>> +        goto err_out_unlock;
>>>
>>> -    /* no need to lock here, as rbd_dev is not registered yet */
>>>        rc = rbd_dev_snap_devs_update(rbd_dev);
>>>        if (rc)
>>> -        goto err_out_bus;
>>> +        goto err_out_unlock;
>>>
>>> -    down_write(&rbd_dev->header_rwsem);
>>>        rc = rbd_header_set_snap(rbd_dev, snap_name);
>>> -    up_write(&rbd_dev->header_rwsem);
>>>        if (rc)
>>> -        goto err_out_bus;
>>> +        goto err_out_unlock;
>>> +
>>> +    up_write(&rbd_dev->header_rwsem);
>>>
>>>        /* Set up the blkdev mapping. */
>>>
>>> @@ -2630,6 +2631,8 @@ static ssize_t rbd_add(struct bus_type *bus,
>>>
>>>        return count;
>>>
>>> +err_out_unlock:
>>> +    up_write(&rbd_dev->header_rwsem);
>>>    err_out_bus:
>>>        /* this will also clean up rest of rbd_dev stuff */
>>>
>>> @@ -2649,6 +2652,7 @@ err_put_id:
>>>            kfree(rbd_dev->pool_name);
>>>        }
>>>        rbd_dev_id_put(rbd_dev);
>>> +    up_write(&rbd_dev->header_rwsem);
>>>    err_nomem:
>>>        kfree(rbd_dev);
>>>        kfree(options);
>>>
>>
>>
>>