| Message ID | 1406191369-6746-7-git-send-email-ilya.dryomov@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <elder@linaro.org> > Signed-off-by: Ilya Dryomov <ilya.dryomov@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 24, 2014 at 5:25 PM, Alex Elder <elder@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 24, 2014 at 5:46 PM, Ilya Dryomov <ilya.dryomov@inktank.com> wrote: > On Thu, Jul 24, 2014 at 5:25 PM, Alex Elder <elder@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/24/2014 08:46 AM, Ilya Dryomov wrote: > On Thu, Jul 24, 2014 at 5:25 PM, Alex Elder <elder@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() or rbd_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 <elder@linaro.org> > > Thanks, > > Ilya > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/24/2014 10:10 AM, Ilya Dryomov wrote: > On Thu, Jul 24, 2014 at 5:46 PM, Ilya Dryomov <ilya.dryomov@inktank.com> wrote: >> On Thu, Jul 24, 2014 at 5:25 PM, Alex Elder <elder@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
There is no sense in trying to update the mapping size before it's even been set. Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> --- drivers/block/rbd.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)