Re: [PATCH 6/8] rbd: update mapping size only on refresh

[Alex Elder]

On 07/24/2014 10:10 AM, Ilya Dryomov wrote:
 On Thu, Jul 24, 2014 at 5:46 PM, Ilya Dryomov ilya.dryo...@inktank.com 
 wrote:
 On Thu, Jul 24, 2014 at 5:25 PM, Alex Elder el...@ieee.org wrote:
 On 07/24/2014 03:42 AM, Ilya Dryomov wrote:
 There is no sense in trying to update the mapping size before it's even
 been set.

 It took me a bit to follow this.  But basically there is
 no mapping unless it's mapped.  So previously this was
 updating the mapping information even for unmapped
 parent (or other) images.  There's no need to update
 the mapping size for a snapshot--it'll never change.

 Is that right?  If not, please advise; otherwise:

 No.  Previously it was updating the mapping size both on the inital map
 and on refresh (of the base image).  Doing that on the inital map
 doesn't make sense: not only struct rbd_mapping fields aren't properly
 initialized at that point - rbd_dev_mapping_set() is called much later
 in the map sequence, snap_id isn't initialized either (at least in the
 format 2 case, I haven't looked too closely at the format 1 case).  And
 just in general, trying to update something before it had a chance to
 change is bogus..
 
 BTW, while we are on the subject of struct rbd_mapping, is there any
 particular reason to keep around both the base image values and actual
 mapping values?  I am tempted to change the mapping sequence so that 1)
 snap context is read in immediately after watch setup, before anything
 else is done, 2) supplied snap name is resolved into an id and 3) the
 actual (i.e. based on snap_id) mapping size, features, parent_overlap,
 etc are read in directly into struct rbd_image_header.  That would
 allow to rip struct rbd_mapping entirely and make the code more clear.

The rbd_mapping structure started out well-intentioned but
over time it seems clear it hasn't added the value it was
intended to add.  Here's where it got created:
f84344f rbd: separate mapping info in rbd_dev
The only original field that survives is read_only.  There's
no harm at all in just moving the fields in that structure
out into the rbd_device structure.

Preserving the base image size in the header structure is
an artifact of the version 1 code, where it held the last
version of the on-disk header data.  The version 2 code
maintains it for consistency.

You could eliminate that I suppose.  I think the result
would require rbd_header_from_disk() to know about the
mapping rather than doing a simple conversion of data
from one form to another.

I say try it, and if you like the result I probably will too...

-Alex


 Thanks,
 
 Ilya
 

--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/8] rbd: update mapping size only on refresh

[Ilya Dryomov]

There is no sense in trying to update the mapping size before it's even
been set.

Signed-off-by: Ilya Dryomov ilya.dryo...@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 5dd6f5057ef4..16f388f2960b 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -971,12 +971,6 @@ static int rbd_header_from_disk(struct rbd_device *rbd_dev,
header-snap_names = snap_names;
header-snap_sizes = snap_sizes;
 
-   /* Make sure mapping size is consistent with header info */
-
-   if (rbd_dev-spec-snap_id == CEPH_NOSNAP || first_time)
-   if (rbd_dev-mapping.size != header-image_size)
-   rbd_dev-mapping.size = header-image_size;
-
return 0;
 out_2big:
ret = -EIO;
@@ -3520,9 +3514,14 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev)
 
ret = rbd_dev_header_info(rbd_dev);
 
-   /* If it's a mapped snapshot, validate its EXISTS flag */
+   if (rbd_dev-spec-snap_id == CEPH_NOSNAP) {
+   if (rbd_dev-mapping.size != rbd_dev-header.image_size)
+   rbd_dev-mapping.size = rbd_dev-header.image_size;
+   } else {
+   /* validate mapped snapshot's EXISTS flag */
+   rbd_exists_validate(rbd_dev);
+   }
 
-   rbd_exists_validate(rbd_dev);
up_write(rbd_dev-header_rwsem);
 
if (mapping_size != rbd_dev-mapping.size) {
@@ -4505,10 +4504,6 @@ static int rbd_dev_v2_header_info(struct rbd_device 
*rbd_dev)
is EXPERIMENTAL!);
}
 
-   if (rbd_dev-spec-snap_id == CEPH_NOSNAP)
-   if (rbd_dev-mapping.size != rbd_dev-header.image_size)
-   rbd_dev-mapping.size = rbd_dev-header.image_size;
-
ret = rbd_dev_v2_snap_context(rbd_dev);
dout(rbd_dev_v2_snap_context returned %d\n, ret);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] rbd: update mapping size only on refresh

[Alex Elder]

On 07/24/2014 03:42 AM, Ilya Dryomov wrote:
 There is no sense in trying to update the mapping size before it's even
 been set.

It took me a bit to follow this.  But basically there is
no mapping unless it's mapped.  So previously this was
updating the mapping information even for unmapped
parent (or other) images.  There's no need to update
the mapping size for a snapshot--it'll never change.

Is that right?  If not, please advise; otherwise:

Reviewed-by: Alex Elder el...@linaro.org

 Signed-off-by: Ilya Dryomov ilya.dryo...@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 5dd6f5057ef4..16f388f2960b 100644
 --- a/drivers/block/rbd.c
 +++ b/drivers/block/rbd.c
 @@ -971,12 +971,6 @@ static int rbd_header_from_disk(struct rbd_device 
 *rbd_dev,
   header-snap_names = snap_names;
   header-snap_sizes = snap_sizes;
  
 - /* Make sure mapping size is consistent with header info */
 -
 - if (rbd_dev-spec-snap_id == CEPH_NOSNAP || first_time)
 - if (rbd_dev-mapping.size != header-image_size)
 - rbd_dev-mapping.size = header-image_size;
 -
   return 0;
  out_2big:
   ret = -EIO;
 @@ -3520,9 +3514,14 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev)
  
   ret = rbd_dev_header_info(rbd_dev);
  
 - /* If it's a mapped snapshot, validate its EXISTS flag */
 + if (rbd_dev-spec-snap_id == CEPH_NOSNAP) {
 + if (rbd_dev-mapping.size != rbd_dev-header.image_size)
 + rbd_dev-mapping.size = rbd_dev-header.image_size;
 + } else {
 + /* validate mapped snapshot's EXISTS flag */
 + rbd_exists_validate(rbd_dev);
 + }
  
 - rbd_exists_validate(rbd_dev);
   up_write(rbd_dev-header_rwsem);
  
   if (mapping_size != rbd_dev-mapping.size) {
 @@ -4505,10 +4504,6 @@ static int rbd_dev_v2_header_info(struct rbd_device 
 *rbd_dev)
   is EXPERIMENTAL!);
   }
  
 - if (rbd_dev-spec-snap_id == CEPH_NOSNAP)
 - if (rbd_dev-mapping.size != rbd_dev-header.image_size)
 - rbd_dev-mapping.size = rbd_dev-header.image_size;
 -
   ret = rbd_dev_v2_snap_context(rbd_dev);
   dout(rbd_dev_v2_snap_context returned %d\n, ret);
  
 

--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] rbd: update mapping size only on refresh

[Ilya Dryomov]

On Thu, Jul 24, 2014 at 5:25 PM, Alex Elder el...@ieee.org wrote:
 On 07/24/2014 03:42 AM, Ilya Dryomov wrote:
 There is no sense in trying to update the mapping size before it's even
 been set.

 It took me a bit to follow this.  But basically there is
 no mapping unless it's mapped.  So previously this was
 updating the mapping information even for unmapped
 parent (or other) images.  There's no need to update
 the mapping size for a snapshot--it'll never change.

 Is that right?  If not, please advise; otherwise:

No.  Previously it was updating the mapping size both on the inital map
and on refresh (of the base image).  Doing that on the inital map
doesn't make sense: not only struct rbd_mapping fields aren't properly
initialized at that point - rbd_dev_mapping_set() is called much later
in the map sequence, snap_id isn't initialized either (at least in the
format 2 case, I haven't looked too closely at the format 1 case).  And
just in general, trying to update something before it had a chance to
change is bogus..

Thanks,

Ilya
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] rbd: update mapping size only on refresh

[Ilya Dryomov]

On Thu, Jul 24, 2014 at 5:46 PM, Ilya Dryomov ilya.dryo...@inktank.com wrote:
 On Thu, Jul 24, 2014 at 5:25 PM, Alex Elder el...@ieee.org wrote:
 On 07/24/2014 03:42 AM, Ilya Dryomov wrote:
 There is no sense in trying to update the mapping size before it's even
 been set.

 It took me a bit to follow this.  But basically there is
 no mapping unless it's mapped.  So previously this was
 updating the mapping information even for unmapped
 parent (or other) images.  There's no need to update
 the mapping size for a snapshot--it'll never change.

 Is that right?  If not, please advise; otherwise:

 No.  Previously it was updating the mapping size both on the inital map
 and on refresh (of the base image).  Doing that on the inital map
 doesn't make sense: not only struct rbd_mapping fields aren't properly
 initialized at that point - rbd_dev_mapping_set() is called much later
 in the map sequence, snap_id isn't initialized either (at least in the
 format 2 case, I haven't looked too closely at the format 1 case).  And
 just in general, trying to update something before it had a chance to
 change is bogus..

BTW, while we are on the subject of struct rbd_mapping, is there any
particular reason to keep around both the base image values and actual
mapping values?  I am tempted to change the mapping sequence so that 1)
snap context is read in immediately after watch setup, before anything
else is done, 2) supplied snap name is resolved into an id and 3) the
actual (i.e. based on snap_id) mapping size, features, parent_overlap,
etc are read in directly into struct rbd_image_header.  That would
allow to rip struct rbd_mapping entirely and make the code more clear.

Thanks,

Ilya
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] rbd: update mapping size only on refresh

[Alex Elder]

On 07/24/2014 08:46 AM, Ilya Dryomov wrote:
 On Thu, Jul 24, 2014 at 5:25 PM, Alex Elder el...@ieee.org wrote:
 On 07/24/2014 03:42 AM, Ilya Dryomov wrote:
 There is no sense in trying to update the mapping size before it's even
 been set.

 It took me a bit to follow this.  But basically there is
 no mapping unless it's mapped.  So previously this was
 updating the mapping information even for unmapped
 parent (or other) images.  There's no need to update
 the mapping size for a snapshot--it'll never change.

 Is that right?  If not, please advise; otherwise:
 
 No.  Previously it was updating the mapping size both on the inital map
 and on refresh (of the base image).  Doing that on the inital map
 doesn't make sense: not only struct rbd_mapping fields aren't properly
 initialized at that point - rbd_dev_mapping_set() is called much later
 in the map sequence, snap_id isn't initialized either (at least in the
 format 2 case, I haven't looked too closely at the format 1 case).  And
 just in general, trying to update something before it had a chance to
 change is bogus..

OK, now I see it.  Hopefully this makes sense.  Here is how it
was previously structured:

  rbd_add()
do_rbd_add()
 |rbd_dev_image_probe()
 |  rbd_dev_header_info()
 |rbd_dev_v1_header_info()orrbd_dev_v2_header_info()
 |  rbd_header_from_disk()update mapping
 |update mapping
 |  . . .
 |rbd_dev_device_setup()
 |  rbd_dev_mapping_set()

So neither rbd_header_from_disk() nor rbd_dev_v2_header_info()
should be using the values in the rbd_dev-mapping field during
initial image probe, because they have not yet been initialized
at that point.

Meanwhile, the only reason we need to *update* a mapping size
is if we learn it may have changed, which will be covered by a
refresh, so doing so in rbd_dev_refresh() makes sense.  And
as long as you know whether it's mapping the base image you
might as well do the rbd_exists_validate() call conditionally.

OK, this all looks good and makes good sense to me.
Thank you for explaining it.

Reviewed-by: Alex Elder el...@linaro.org


 
 Thanks,
 
 Ilya
 

--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html