[PATCH 0/3] rbd: parent_overlap == 0 fixes

[PATCH 0/3] rbd: parent_overlap == 0 fixes

[Ilya Dryomov]

Hello,

This series fixes what turned out to be an old (v3.10-v3.11) bug, which
can cause things like http://tracker.ceph.com/issues/10352.  Most of
the damage was brought by 3b5cf2a2f174 ("rbd: clean up a few things in
the refresh path") which went into v3.11, but v3.10 wasn't fully OK
either.

Thanks,

                Ilya


Ilya Dryomov (3):
  rbd: fix rbd_dev_parent_get() when parent_overlap == 0
  rbd: drop parent_ref in rbd_dev_unprobe() unconditionally
  rbd: do not treat standalone as flatten

 drivers/block/rbd.c | 55 +++++++++++++++++------------------------------------
 1 file changed, 17 insertions(+), 38 deletions(-)

-- 
1.9.3

[PATCH 1/3] rbd: fix rbd_dev_parent_get() when parent_overlap == 0

[Ilya Dryomov]

The comment for rbd_dev_parent_get() said

    * We must get the reference before checking for the overlap to
    * coordinate properly with zeroing the parent overlap in
    * rbd_dev_v2_parent_info() when an image gets flattened.  We
    * drop it again if there is no overlap.

but the "drop it again if there is no overlap" part was missing from
the implementation.  This lead to absurd parent_ref values for images
with parent_overlap == 0, as parent_ref was incremented for each
img_request and virtually never decremented.

Fix this by leveraging the fact that refresh path calls
rbd_dev_v2_parent_info() under header_rwsem and use it for read in
rbd_dev_parent_get(), instead of messing around with atomics.  Get rid
of barriers in rbd_dev_v2_parent_info() while at it - I don't see what
they'd pair with now and I suspect we are in a pretty miserable
situation as far as proper locking goes regardless.

Cc: stable@vger.kernel.org # 3.11+
Signed-off-by: Ilya Dryomov <idryomov@redhat.com>
---
 drivers/block/rbd.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 31fa00f0d707..2990a1c75159 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2098,32 +2098,26 @@ static void rbd_dev_parent_put(struct rbd_device *rbd_dev)
  * If an image has a non-zero parent overlap, get a reference to its
  * parent.
  *
- * We must get the reference before checking for the overlap to
- * coordinate properly with zeroing the parent overlap in
- * rbd_dev_v2_parent_info() when an image gets flattened.  We
- * drop it again if there is no overlap.
- *
  * Returns true if the rbd device has a parent with a non-zero
  * overlap and a reference for it was successfully taken, or
  * false otherwise.
  */
 static bool rbd_dev_parent_get(struct rbd_device *rbd_dev)
 {
-	int counter;
+	int counter = 0;
 
 	if (!rbd_dev->parent_spec)
 		return false;
 
-	counter = atomic_inc_return_safe(&rbd_dev->parent_ref);
-	if (counter > 0 && rbd_dev->parent_overlap)
-		return true;
-
-	/* Image was flattened, but parent is not yet torn down */
+	down_read(&rbd_dev->header_rwsem);
+	if (rbd_dev->parent_overlap)
+		counter = atomic_inc_return_safe(&rbd_dev->parent_ref);
+	up_read(&rbd_dev->header_rwsem);
 
 	if (counter < 0)
 		rbd_warn(rbd_dev, "parent reference overflow");
 
-	return false;
+	return counter > 0;
 }
 
 /*
@@ -4238,7 +4232,6 @@ static int rbd_dev_v2_parent_info(struct rbd_device *rbd_dev)
 		 */
 		if (rbd_dev->parent_overlap) {
 			rbd_dev->parent_overlap = 0;
-			smp_mb();
 			rbd_dev_parent_put(rbd_dev);
 			pr_info("%s: clone image has been flattened\n",
 				rbd_dev->disk->disk_name);
@@ -4284,7 +4277,6 @@ static int rbd_dev_v2_parent_info(struct rbd_device *rbd_dev)
 	 * treat it specially.
 	 */
 	rbd_dev->parent_overlap = overlap;
-	smp_mb();
 	if (!overlap) {
 
 		/* A null parent_spec indicates it's the initial probe */
-- 
1.9.3

[PATCH 2/3] rbd: drop parent_ref in rbd_dev_unprobe() unconditionally

[Ilya Dryomov]

This effectively reverts the last hunk of 392a9dad7e77 ("rbd: detect
when clone image is flattened").

The problem with parent_overlap != 0 condition is that it's possible
and completely valid to have an image with parent_overlap == 0 whose
parent state needs to be cleaned up on unmap.  The next commit, which
drops the "clone image now standalone" logic, opens up another window
of opportunity to hit this, but even without it

    # cat parent-ref.sh
    #!/bin/bash
    rbd create --image-format 2 --size 1 foo
    rbd snap create foo@snap
    rbd snap protect foo@snap
    rbd clone foo@snap bar
    rbd resize --allow-shrink --size 0 bar
    rbd resize --size 1 bar
    DEV=$(rbd map bar)
    rbd unmap $DEV

leaves rbd_device/rbd_spec/etc and rbd_client along with ceph_client
hanging around.

My thinking behind calling rbd_dev_parent_put() unconditionally is that
there shouldn't be any requests in flight at that point in time as we
are deep into unmap sequence.  Hence, even if rbd_dev_unparent() caused
by flatten is delayed by in-flight requests, it will have finished by
the time we reach rbd_dev_unprobe() caused by unmap, thus turning
unconditional rbd_dev_parent_put() into a no-op.

Fixes: http://tracker.ceph.com/issues/10352

Cc: stable@vger.kernel.org # 3.11+
Signed-off-by: Ilya Dryomov <idryomov@redhat.com>
---
 drivers/block/rbd.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 2990a1c75159..b85d52005a21 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -5075,10 +5075,7 @@ static void rbd_dev_unprobe(struct rbd_device *rbd_dev)
 {
 	struct rbd_image_header	*header;
 
-	/* Drop parent reference unless it's already been done (or none) */
-
-	if (rbd_dev->parent_overlap)
-		rbd_dev_parent_put(rbd_dev);
+	rbd_dev_parent_put(rbd_dev);
 
 	/* Free dynamic fields from the header, then zero it out */
 
-- 
1.9.3

[PATCH 3/3] rbd: do not treat standalone as flatten

[Ilya Dryomov]

If the clone is resized down to 0, it becomes standalone.  If such
resize is carried over while an image is mapped we would detect this
and call rbd_dev_parent_put() which means "let go of all parent state,
including the spec(s) of parent images(s)".  This leads to a mismatch
between "rbd info" and sysfs parent fields, so a fix is in order.

    # rbd create --image-format 2 --size 1 foo
    # rbd snap create foo@snap
    # rbd snap protect foo@snap
    # rbd clone foo@snap bar
    # DEV=$(rbd map bar)
    # rbd resize --allow-shrink --size 0 bar
    # rbd resize --size 1 bar
    # rbd info bar | grep parent
            parent: rbd/foo@snap

Before:

    # cat /sys/bus/rbd/devices/0/parent
    (no parent image)

After:

    # cat /sys/bus/rbd/devices/0/parent
    pool_id 0
    pool_name rbd
    image_id 10056b8b4567
    image_name foo
    snap_id 2
    snap_name snap
    overlap 0

Signed-off-by: Ilya Dryomov <idryomov@redhat.com>
---
 drivers/block/rbd.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index b85d52005a21..e818c2a6ffb1 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4273,32 +4273,22 @@ static int rbd_dev_v2_parent_info(struct rbd_device *rbd_dev)
 	}
 
 	/*
-	 * We always update the parent overlap.  If it's zero we
-	 * treat it specially.
+	 * We always update the parent overlap.  If it's zero we issue
+	 * a warning, as we will proceed as if there was no parent.
 	 */
-	rbd_dev->parent_overlap = overlap;
 	if (!overlap) {
-
-		/* A null parent_spec indicates it's the initial probe */
-
 		if (parent_spec) {
-			/*
-			 * The overlap has become zero, so the clone
-			 * must have been resized down to 0 at some
-			 * point.  Treat this the same as a flatten.
-			 */
-			rbd_dev_parent_put(rbd_dev);
-			pr_info("%s: clone image now standalone\n",
-				rbd_dev->disk->disk_name);
+			/* refresh, careful to warn just once */
+			if (rbd_dev->parent_overlap)
+				rbd_warn(rbd_dev,
+				    "clone now standalone (overlap became 0)");
 		} else {
-			/*
-			 * For the initial probe, if we find the
-			 * overlap is zero we just pretend there was
-			 * no parent image.
-			 */
-			rbd_warn(rbd_dev, "ignoring parent with overlap 0");
+			/* initial probe */
+			rbd_warn(rbd_dev, "clone is standalone (overlap 0)");
 		}
 	}
+	rbd_dev->parent_overlap = overlap;
+
 out:
 	ret = 0;
 out_err:
-- 
1.9.3

Re: [PATCH 1/3] rbd: fix rbd_dev_parent_get() when parent_overlap == 0

[Josh Durgin]

On 01/20/2015 04:41 AM, Ilya Dryomov wrote:
> The comment for rbd_dev_parent_get() said
>
>      * We must get the reference before checking for the overlap to
>      * coordinate properly with zeroing the parent overlap in
>      * rbd_dev_v2_parent_info() when an image gets flattened.  We
>      * drop it again if there is no overlap.
>
> but the "drop it again if there is no overlap" part was missing from
> the implementation.  This lead to absurd parent_ref values for images
> with parent_overlap == 0, as parent_ref was incremented for each
> img_request and virtually never decremented.
>
> Fix this by leveraging the fact that refresh path calls
> rbd_dev_v2_parent_info() under header_rwsem and use it for read in
> rbd_dev_parent_get(), instead of messing around with atomics.  Get rid
> of barriers in rbd_dev_v2_parent_info() while at it - I don't see what
> they'd pair with now and I suspect we are in a pretty miserable
> situation as far as proper locking goes regardless.

Yeah, looks like we need some refactoring to read parent_overlap safely
in the I/O path in a few places.

Reviewed-by: Josh Durgin <jdurgin@redhat.com>

> Cc: stable@vger.kernel.org # 3.11+
> Signed-off-by: Ilya Dryomov <idryomov@redhat.com>
> ---
>   drivers/block/rbd.c | 20 ++++++--------------
>   1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 31fa00f0d707..2990a1c75159 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2098,32 +2098,26 @@ static void rbd_dev_parent_put(struct rbd_device *rbd_dev)
>    * If an image has a non-zero parent overlap, get a reference to its
>    * parent.
>    *
> - * We must get the reference before checking for the overlap to
> - * coordinate properly with zeroing the parent overlap in
> - * rbd_dev_v2_parent_info() when an image gets flattened.  We
> - * drop it again if there is no overlap.
> - *
>    * Returns true if the rbd device has a parent with a non-zero
>    * overlap and a reference for it was successfully taken, or
>    * false otherwise.
>    */
>   static bool rbd_dev_parent_get(struct rbd_device *rbd_dev)
>   {
> -	int counter;
> +	int counter = 0;
>
>   	if (!rbd_dev->parent_spec)
>   		return false;
>
> -	counter = atomic_inc_return_safe(&rbd_dev->parent_ref);
> -	if (counter > 0 && rbd_dev->parent_overlap)
> -		return true;
> -
> -	/* Image was flattened, but parent is not yet torn down */
> +	down_read(&rbd_dev->header_rwsem);
> +	if (rbd_dev->parent_overlap)
> +		counter = atomic_inc_return_safe(&rbd_dev->parent_ref);
> +	up_read(&rbd_dev->header_rwsem);
>
>   	if (counter < 0)
>   		rbd_warn(rbd_dev, "parent reference overflow");
>
> -	return false;
> +	return counter > 0;
>   }
>
>   /*
> @@ -4238,7 +4232,6 @@ static int rbd_dev_v2_parent_info(struct rbd_device *rbd_dev)
>   		 */
>   		if (rbd_dev->parent_overlap) {
>   			rbd_dev->parent_overlap = 0;
> -			smp_mb();
>   			rbd_dev_parent_put(rbd_dev);
>   			pr_info("%s: clone image has been flattened\n",
>   				rbd_dev->disk->disk_name);
> @@ -4284,7 +4277,6 @@ static int rbd_dev_v2_parent_info(struct rbd_device *rbd_dev)
>   	 * treat it specially.
>   	 */
>   	rbd_dev->parent_overlap = overlap;
> -	smp_mb();
>   	if (!overlap) {
>
>   		/* A null parent_spec indicates it's the initial probe */
>

Re: [PATCH 2/3] rbd: drop parent_ref in rbd_dev_unprobe() unconditionally

[Josh Durgin]

On 01/20/2015 04:41 AM, Ilya Dryomov wrote:
> This effectively reverts the last hunk of 392a9dad7e77 ("rbd: detect
> when clone image is flattened").
>
> The problem with parent_overlap != 0 condition is that it's possible
> and completely valid to have an image with parent_overlap == 0 whose
> parent state needs to be cleaned up on unmap.  The next commit, which
> drops the "clone image now standalone" logic, opens up another window
> of opportunity to hit this, but even without it
>
>      # cat parent-ref.sh
>      #!/bin/bash
>      rbd create --image-format 2 --size 1 foo
>      rbd snap create foo@snap
>      rbd snap protect foo@snap
>      rbd clone foo@snap bar
>      rbd resize --allow-shrink --size 0 bar
>      rbd resize --size 1 bar
>      DEV=$(rbd map bar)
>      rbd unmap $DEV
>
> leaves rbd_device/rbd_spec/etc and rbd_client along with ceph_client
> hanging around.
>
> My thinking behind calling rbd_dev_parent_put() unconditionally is that
> there shouldn't be any requests in flight at that point in time as we
> are deep into unmap sequence.  Hence, even if rbd_dev_unparent() caused
> by flatten is delayed by in-flight requests, it will have finished by
> the time we reach rbd_dev_unprobe() caused by unmap, thus turning
> unconditional rbd_dev_parent_put() into a no-op.
>
> Fixes: http://tracker.ceph.com/issues/10352
>
> Cc: stable@vger.kernel.org # 3.11+
> Signed-off-by: Ilya Dryomov <idryomov@redhat.com>

Reviewed-by: Josh Durgin <jdurgin@redhat.com>

Re: [PATCH 3/3] rbd: do not treat standalone as flatten

[Josh Durgin]

On 01/20/2015 04:41 AM, Ilya Dryomov wrote:
> If the clone is resized down to 0, it becomes standalone.  If such
> resize is carried over while an image is mapped we would detect this
> and call rbd_dev_parent_put() which means "let go of all parent state,
> including the spec(s) of parent images(s)".  This leads to a mismatch
> between "rbd info" and sysfs parent fields, so a fix is in order.
>
>      # rbd create --image-format 2 --size 1 foo
>      # rbd snap create foo@snap
>      # rbd snap protect foo@snap
>      # rbd clone foo@snap bar
>      # DEV=$(rbd map bar)
>      # rbd resize --allow-shrink --size 0 bar
>      # rbd resize --size 1 bar
>      # rbd info bar | grep parent
>              parent: rbd/foo@snap
>
> Before:
>
>      # cat /sys/bus/rbd/devices/0/parent
>      (no parent image)
>
> After:
>
>      # cat /sys/bus/rbd/devices/0/parent
>      pool_id 0
>      pool_name rbd
>      image_id 10056b8b4567
>      image_name foo
>      snap_id 2
>      snap_name snap
>      overlap 0
>
> Signed-off-by: Ilya Dryomov <idryomov@redhat.com>

Reviewed-by: Josh Durgin <jdurgin@redhat.com>

Re: [PATCH 1/3] rbd: fix rbd_dev_parent_get() when parent_overlap == 0

[Alex Elder]

On 01/20/2015 06:41 AM, Ilya Dryomov wrote:
> The comment for rbd_dev_parent_get() said
> 
>     * We must get the reference before checking for the overlap to
>     * coordinate properly with zeroing the parent overlap in
>     * rbd_dev_v2_parent_info() when an image gets flattened.  We
>     * drop it again if there is no overlap.
> 
> but the "drop it again if there is no overlap" part was missing from
> the implementation.  This lead to absurd parent_ref values for images
> with parent_overlap == 0, as parent_ref was incremented for each
> img_request and virtually never decremented.

You're right about this.  If the image had a parent with no
overlap this would leak a reference to the parent image.  The
code should have said:

	counter = atomic_inc_return_safe(&rbd_dev->parent_ref);
	if (counter > 0) {
		if (rbd_dev->parent_overlap)
			return true;
		atomic_dec(&rbd_dev->parent_ref);
	} else if (counter < 0) {
 		rbd_warn(rbd_dev, "parent reference overflow");
	}

> Fix this by leveraging the fact that refresh path calls
> rbd_dev_v2_parent_info() under header_rwsem and use it for read in
> rbd_dev_parent_get(), instead of messing around with atomics.  Get rid
> of barriers in rbd_dev_v2_parent_info() while at it - I don't see what
> they'd pair with now and I suspect we are in a pretty miserable
> situation as far as proper locking goes regardless.

The point of the memory barrier was to ensure that when parent_overlap
gets zeroed, this code sees the zero rather than the old non-zero
value.  The atomic_inc_return_safe() call has an implicit memory
barrier to match the smp_mb() call.  It allowed the synchronization
to occur without the use of a lock.

We're trying to atomically determine whether an image request needs
to be marked as layered, to know how to handle ENOENT on parent reads.
If it is a write to an image with a parent having a non-zero overlap,
it's layered, otherwise we can treat it as a simple request.

I think in this particular case, this is just an optimization,
trying very hard to avoid having to do layered image handling
if the parent has become flattened.  I think that even if it
got old information (suggesting non-zero overlap) things would
behave correctly, just less efficiently.

Using the semaphore adds a lock to this path and therefore
implements whatever barriers are being removed.  I'm not
sure how often this is hit--maybe the optimization isn't
buying much after all.

I am getting a little rusty on some of details of what
precisely happens when a layered image gets flattened.
But I think this looks OK.  Maybe just watch for small
(perhaps insignificant) performance regressions with
this change in place...

Reviewed-by: Alex Elder <elder@linaro.org>

> Cc: stable@vger.kernel.org # 3.11+
> Signed-off-by: Ilya Dryomov <idryomov@redhat.com>
> ---
>  drivers/block/rbd.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 31fa00f0d707..2990a1c75159 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2098,32 +2098,26 @@ static void rbd_dev_parent_put(struct rbd_device *rbd_dev)
>   * If an image has a non-zero parent overlap, get a reference to its
>   * parent.
>   *
> - * We must get the reference before checking for the overlap to
> - * coordinate properly with zeroing the parent overlap in
> - * rbd_dev_v2_parent_info() when an image gets flattened.  We
> - * drop it again if there is no overlap.
> - *
>   * Returns true if the rbd device has a parent with a non-zero
>   * overlap and a reference for it was successfully taken, or
>   * false otherwise.
>   */
>  static bool rbd_dev_parent_get(struct rbd_device *rbd_dev)
>  {
> -	int counter;
> +	int counter = 0;
>  
>  	if (!rbd_dev->parent_spec)
>  		return false;
>  
> -	counter = atomic_inc_return_safe(&rbd_dev->parent_ref);
> -	if (counter > 0 && rbd_dev->parent_overlap)
> -		return true;
> -
> -	/* Image was flattened, but parent is not yet torn down */
> +	down_read(&rbd_dev->header_rwsem);
> +	if (rbd_dev->parent_overlap)
> +		counter = atomic_inc_return_safe(&rbd_dev->parent_ref);
> +	up_read(&rbd_dev->header_rwsem);
>  
>  	if (counter < 0)
>  		rbd_warn(rbd_dev, "parent reference overflow");
>  
> -	return false;
> +	return counter > 0;
>  }
>  
>  /*
> @@ -4238,7 +4232,6 @@ static int rbd_dev_v2_parent_info(struct rbd_device *rbd_dev)
>  		 */
>  		if (rbd_dev->parent_overlap) {
>  			rbd_dev->parent_overlap = 0;
> -			smp_mb();
>  			rbd_dev_parent_put(rbd_dev);
>  			pr_info("%s: clone image has been flattened\n",
>  				rbd_dev->disk->disk_name);
> @@ -4284,7 +4277,6 @@ static int rbd_dev_v2_parent_info(struct rbd_device *rbd_dev)
>  	 * treat it specially.
>  	 */
>  	rbd_dev->parent_overlap = overlap;
> -	smp_mb();
>  	if (!overlap) {
>  
>  		/* A null parent_spec indicates it's the initial probe */
> 

Re: [PATCH 2/3] rbd: drop parent_ref in rbd_dev_unprobe() unconditionally

[Alex Elder]

On 01/20/2015 06:41 AM, Ilya Dryomov wrote:
> This effectively reverts the last hunk of 392a9dad7e77 ("rbd: detect
> when clone image is flattened").
> 
> The problem with parent_overlap != 0 condition is that it's possible
> and completely valid to have an image with parent_overlap == 0 whose
> parent state needs to be cleaned up on unmap.  The next commit, which
> drops the "clone image now standalone" logic, opens up another window
> of opportunity to hit this, but even without it
> 
>     # cat parent-ref.sh
>     #!/bin/bash
>     rbd create --image-format 2 --size 1 foo
>     rbd snap create foo@snap
>     rbd snap protect foo@snap
>     rbd clone foo@snap bar
>     rbd resize --allow-shrink --size 0 bar
>     rbd resize --size 1 bar
>     DEV=$(rbd map bar)
>     rbd unmap $DEV
> 
> leaves rbd_device/rbd_spec/etc and rbd_client along with ceph_client
> hanging around.

I'm not sure why the last reference to the parent
doesn't get dropped (and state cleaned up) as soon
as the overlap becomes 0.  I suspect it's the original
reference taken when there's a parent, we don't get
rid of it until it's torn down.  (I think we should.)

It seems to me the test here should be for a non-null
parent_spec pointer rather than non-zero parent_overlap.
And that's done inside rbd_dev_parent_put(), so your
change looks reasonable to me.

Reviewed-by: Alex Elder <elder@linaro.org>

> 
> My thinking behind calling rbd_dev_parent_put() unconditionally is that
> there shouldn't be any requests in flight at that point in time as we
> are deep into unmap sequence.  Hence, even if rbd_dev_unparent() caused
> by flatten is delayed by in-flight requests, it will have finished by
> the time we reach rbd_dev_unprobe() caused by unmap, thus turning
> unconditional rbd_dev_parent_put() into a no-op.
> 
> Fixes: http://tracker.ceph.com/issues/10352
> 
> Cc: stable@vger.kernel.org # 3.11+
> Signed-off-by: Ilya Dryomov <idryomov@redhat.com>
> ---
>  drivers/block/rbd.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 2990a1c75159..b85d52005a21 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -5075,10 +5075,7 @@ static void rbd_dev_unprobe(struct rbd_device *rbd_dev)
>  {
>  	struct rbd_image_header	*header;
>  
> -	/* Drop parent reference unless it's already been done (or none) */
> -
> -	if (rbd_dev->parent_overlap)
> -		rbd_dev_parent_put(rbd_dev);
> +	rbd_dev_parent_put(rbd_dev);
>  
>  	/* Free dynamic fields from the header, then zero it out */
>  
> 

Re: [PATCH 3/3] rbd: do not treat standalone as flatten

[Alex Elder]

On 01/20/2015 06:41 AM, Ilya Dryomov wrote:
> If the clone is resized down to 0, it becomes standalone.  If such
> resize is carried over while an image is mapped we would detect this
> and call rbd_dev_parent_put() which means "let go of all parent state,
> including the spec(s) of parent images(s)".  This leads to a mismatch
> between "rbd info" and sysfs parent fields, so a fix is in order.
> 
>     # rbd create --image-format 2 --size 1 foo
>     # rbd snap create foo@snap
>     # rbd snap protect foo@snap
>     # rbd clone foo@snap bar
>     # DEV=$(rbd map bar)
>     # rbd resize --allow-shrink --size 0 bar
>     # rbd resize --size 1 bar
>     # rbd info bar | grep parent
>             parent: rbd/foo@snap
> 
> Before:
> 
>     # cat /sys/bus/rbd/devices/0/parent
>     (no parent image)
> 
> After:
> 
>     # cat /sys/bus/rbd/devices/0/parent
>     pool_id 0
>     pool_name rbd
>     image_id 10056b8b4567
>     image_name foo
>     snap_id 2
>     snap_name snap
>     overlap 0
> 
> Signed-off-by: Ilya Dryomov <idryomov@redhat.com>

Hmm.  Interesting.

I think that a parent with an overlap of 0 is of no
real use.  So in the last patch I was suggesting it
should just go away.

But now, looking at it from this perspective, the fact
that an image *came from* a particular parent, but which
has no more overlap, could be useful information.  The
parent shouldn't simply go away without the user requesting
that.

I haven't completely followed through the logic of keeping
the reference around but I understand what you're doing and
it looks OK to me.

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
>  drivers/block/rbd.c | 30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index b85d52005a21..e818c2a6ffb1 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -4273,32 +4273,22 @@ static int rbd_dev_v2_parent_info(struct rbd_device *rbd_dev)
>  	}
>  
>  	/*
> -	 * We always update the parent overlap.  If it's zero we
> -	 * treat it specially.
> +	 * We always update the parent overlap.  If it's zero we issue
> +	 * a warning, as we will proceed as if there was no parent.
>  	 */
> -	rbd_dev->parent_overlap = overlap;
>  	if (!overlap) {
> -
> -		/* A null parent_spec indicates it's the initial probe */
> -
>  		if (parent_spec) {
> -			/*
> -			 * The overlap has become zero, so the clone
> -			 * must have been resized down to 0 at some
> -			 * point.  Treat this the same as a flatten.
> -			 */
> -			rbd_dev_parent_put(rbd_dev);
> -			pr_info("%s: clone image now standalone\n",
> -				rbd_dev->disk->disk_name);
> +			/* refresh, careful to warn just once */
> +			if (rbd_dev->parent_overlap)
> +				rbd_warn(rbd_dev,
> +				    "clone now standalone (overlap became 0)");
>  		} else {
> -			/*
> -			 * For the initial probe, if we find the
> -			 * overlap is zero we just pretend there was
> -			 * no parent image.
> -			 */
> -			rbd_warn(rbd_dev, "ignoring parent with overlap 0");
> +			/* initial probe */
> +			rbd_warn(rbd_dev, "clone is standalone (overlap 0)");
>  		}
>  	}
> +	rbd_dev->parent_overlap = overlap;
> +
>  out:
>  	ret = 0;
>  out_err:
>