| Message ID | 1393860533-2063-6-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote : > Implement this function in the same way as raw_bsd does: Acknowledge > that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID > and BDRV_BLOCK_DATA and derive the offset directly from the sector > index) and add BDRV_BLOCK_RAW to the returned value. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/json.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/block/json.c b/block/json.c > index a2f4691..7392802 100644 > --- a/block/json.c > +++ b/block/json.c > @@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs, > return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags); > } > > +static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs, > + int64_t sector_num, > + int nb_sectors, int *pnum) > +{ > + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | > + (sector_num << BDRV_SECTOR_BITS); > +} I don't understand what is the selling point of this method instead of calling bdrv_co_get_block_status on bs->file. Some information risk to be lost and it does look like magic. Best regards Benoît > + > static void json_invalidate_cache(BlockDriverState *bs) > { > return bdrv_invalidate_cache(bs->file); > @@ -159,6 +167,7 @@ static BlockDriver bdrv_json = { > .bdrv_aio_discard = json_aio_discard, > > .bdrv_co_write_zeroes = json_co_write_zeroes, > + .bdrv_co_get_block_status = json_co_get_block_status, > > .bdrv_invalidate_cache = json_invalidate_cache, > > -- > 1.9.0 > >
On 05.03.2014 17:11, Benoît Canet wrote: > The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote : >> Implement this function in the same way as raw_bsd does: Acknowledge >> that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID >> and BDRV_BLOCK_DATA and derive the offset directly from the sector >> index) and add BDRV_BLOCK_RAW to the returned value. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/json.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/block/json.c b/block/json.c >> index a2f4691..7392802 100644 >> --- a/block/json.c >> +++ b/block/json.c >> @@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs, >> return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags); >> } >> >> +static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs, >> + int64_t sector_num, >> + int nb_sectors, int *pnum) >> +{ >> + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | >> + (sector_num << BDRV_SECTOR_BITS); >> +} > I don't understand what is the selling point of this method instead of calling > bdrv_co_get_block_status on bs->file. > Some information risk to be lost and it does look like magic. This is the same what "raw" does. It just is more meaningful: This way, this function does not pretend to provide the blocks itself but instead tells the truth; that is, the blocks are provided by an underlying BDS (bs->file). I wasn't really sure what to do myself. Generally, this driver is actually meant to pretend that it provides the blocks itself. On the other hand, I tried to imitate the behavior or "raw", since this is something I can hope to be approximately correct. Also, as I've said before, the value returned here is in fact at least technically correct. Max > Best regards > > Benoît > >> + >> static void json_invalidate_cache(BlockDriverState *bs) >> { >> return bdrv_invalidate_cache(bs->file); >> @@ -159,6 +167,7 @@ static BlockDriver bdrv_json = { >> .bdrv_aio_discard = json_aio_discard, >> >> .bdrv_co_write_zeroes = json_co_write_zeroes, >> + .bdrv_co_get_block_status = json_co_get_block_status, >> >> .bdrv_invalidate_cache = json_invalidate_cache, >> >> -- >> 1.9.0 >> >>
The Wednesday 05 Mar 2014 à 21:10:03 (+0100), Max Reitz wrote : > On 05.03.2014 17:11, Benoît Canet wrote: > >The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote : > >>Implement this function in the same way as raw_bsd does: Acknowledge > >>that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID > >>and BDRV_BLOCK_DATA and derive the offset directly from the sector > >>index) and add BDRV_BLOCK_RAW to the returned value. > >> > >>Signed-off-by: Max Reitz <mreitz@redhat.com> > >>--- > >> block/json.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >>diff --git a/block/json.c b/block/json.c > >>index a2f4691..7392802 100644 > >>--- a/block/json.c > >>+++ b/block/json.c > >>@@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs, > >> return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags); > >> } > >>+static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs, > >>+ int64_t sector_num, > >>+ int nb_sectors, int *pnum) > >>+{ > >>+ return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | > >>+ (sector_num << BDRV_SECTOR_BITS); > >>+} > >I don't understand what is the selling point of this method instead of calling > >bdrv_co_get_block_status on bs->file. > >Some information risk to be lost and it does look like magic. > > This is the same what "raw" does. It just is more meaningful: This > way, this function does not pretend to provide the blocks itself but > instead tells the truth; that is, the blocks are provided by an > underlying BDS (bs->file). > > I wasn't really sure what to do myself. Generally, this driver is > actually meant to pretend that it provides the blocks itself. On the > other hand, I tried to imitate the behavior or "raw", since this is > something I can hope to be approximately correct. Also, as I've said > before, the value returned here is in fact at least technically > correct. The raw_bsd driver have an additional *pnum = nb_sectors; at the begining of the function. Did you left it on purpose ? Best regards Benoît > > > Max > > >Best regards > > > >Benoît > > > >>+ > >> static void json_invalidate_cache(BlockDriverState *bs) > >> { > >> return bdrv_invalidate_cache(bs->file); > >>@@ -159,6 +167,7 @@ static BlockDriver bdrv_json = { > >> .bdrv_aio_discard = json_aio_discard, > >> .bdrv_co_write_zeroes = json_co_write_zeroes, > >>+ .bdrv_co_get_block_status = json_co_get_block_status, > >> .bdrv_invalidate_cache = json_invalidate_cache, > >>-- > >>1.9.0 > >> > >> >
On 05.03.2014 21:41, Benoît Canet wrote: > The Wednesday 05 Mar 2014 à 21:10:03 (+0100), Max Reitz wrote : >> On 05.03.2014 17:11, Benoît Canet wrote: >>> The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote : >>>> Implement this function in the same way as raw_bsd does: Acknowledge >>>> that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID >>>> and BDRV_BLOCK_DATA and derive the offset directly from the sector >>>> index) and add BDRV_BLOCK_RAW to the returned value. >>>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> block/json.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/block/json.c b/block/json.c >>>> index a2f4691..7392802 100644 >>>> --- a/block/json.c >>>> +++ b/block/json.c >>>> @@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs, >>>> return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags); >>>> } >>>> +static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs, >>>> + int64_t sector_num, >>>> + int nb_sectors, int *pnum) >>>> +{ >>>> + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | >>>> + (sector_num << BDRV_SECTOR_BITS); >>>> +} >>> I don't understand what is the selling point of this method instead of calling >>> bdrv_co_get_block_status on bs->file. >>> Some information risk to be lost and it does look like magic. >> This is the same what "raw" does. It just is more meaningful: This >> way, this function does not pretend to provide the blocks itself but >> instead tells the truth; that is, the blocks are provided by an >> underlying BDS (bs->file). >> >> I wasn't really sure what to do myself. Generally, this driver is >> actually meant to pretend that it provides the blocks itself. On the >> other hand, I tried to imitate the behavior or "raw", since this is >> something I can hope to be approximately correct. Also, as I've said >> before, the value returned here is in fact at least technically >> correct. > The raw_bsd driver have an additional *pnum = nb_sectors; at the begining of the > function. Did you left it on purpose ? Oops, no, I did not. Okay, if I even fail at copying code, the argument about "raw" being at least probably correct isn't worth anything here, I guess. ;-) Max > Best regards > > Benoît >> >> Max >> >>> Best regards >>> >>> Benoît >>> >>>> + >>>> static void json_invalidate_cache(BlockDriverState *bs) >>>> { >>>> return bdrv_invalidate_cache(bs->file); >>>> @@ -159,6 +167,7 @@ static BlockDriver bdrv_json = { >>>> .bdrv_aio_discard = json_aio_discard, >>>> .bdrv_co_write_zeroes = json_co_write_zeroes, >>>> + .bdrv_co_get_block_status = json_co_get_block_status, >>>> .bdrv_invalidate_cache = json_invalidate_cache, >>>> -- >>>> 1.9.0 >>>> >>>>
The Wednesday 05 Mar 2014 à 21:44:57 (+0100), Max Reitz wrote : > On 05.03.2014 21:41, Benoît Canet wrote: > >The Wednesday 05 Mar 2014 à 21:10:03 (+0100), Max Reitz wrote : > >>On 05.03.2014 17:11, Benoît Canet wrote: > >>>The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote : > >>>>Implement this function in the same way as raw_bsd does: Acknowledge > >>>>that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID > >>>>and BDRV_BLOCK_DATA and derive the offset directly from the sector > >>>>index) and add BDRV_BLOCK_RAW to the returned value. > >>>> > >>>>Signed-off-by: Max Reitz <mreitz@redhat.com> > >>>>--- > >>>> block/json.c | 9 +++++++++ > >>>> 1 file changed, 9 insertions(+) > >>>> > >>>>diff --git a/block/json.c b/block/json.c > >>>>index a2f4691..7392802 100644 > >>>>--- a/block/json.c > >>>>+++ b/block/json.c > >>>>@@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs, > >>>> return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags); > >>>> } > >>>>+static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs, > >>>>+ int64_t sector_num, > >>>>+ int nb_sectors, int *pnum) > >>>>+{ > >>>>+ return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | > >>>>+ (sector_num << BDRV_SECTOR_BITS); > >>>>+} > >>>I don't understand what is the selling point of this method instead of calling > >>>bdrv_co_get_block_status on bs->file. > >>>Some information risk to be lost and it does look like magic. > >>This is the same what "raw" does. It just is more meaningful: This > >>way, this function does not pretend to provide the blocks itself but > >>instead tells the truth; that is, the blocks are provided by an > >>underlying BDS (bs->file). > >> > >>I wasn't really sure what to do myself. Generally, this driver is > >>actually meant to pretend that it provides the blocks itself. On the > >>other hand, I tried to imitate the behavior or "raw", since this is > >>something I can hope to be approximately correct. Also, as I've said > >>before, the value returned here is in fact at least technically > >>correct. > >The raw_bsd driver have an additional *pnum = nb_sectors; at the begining of the > >function. Did you left it on purpose ? > > Oops, no, I did not. Okay, if I even fail at copying code, the > argument about "raw" being at least probably correct isn't worth > anything here, I guess. ;-) In fact I wonder i posix_raw act this way because one of the BDRV it does wrap have simply no clue on how to answer to this request. Best regards Benoît > > Max > > >Best regards > > > >Benoît > >> > >>Max > >> > >>>Best regards > >>> > >>>Benoît > >>> > >>>>+ > >>>> static void json_invalidate_cache(BlockDriverState *bs) > >>>> { > >>>> return bdrv_invalidate_cache(bs->file); > >>>>@@ -159,6 +167,7 @@ static BlockDriver bdrv_json = { > >>>> .bdrv_aio_discard = json_aio_discard, > >>>> .bdrv_co_write_zeroes = json_co_write_zeroes, > >>>>+ .bdrv_co_get_block_status = json_co_get_block_status, > >>>> .bdrv_invalidate_cache = json_invalidate_cache, > >>>>-- > >>>>1.9.0 > >>>> > >>>> >
On 06.03.2014 00:22, Benoît Canet wrote: > The Wednesday 05 Mar 2014 à 21:44:57 (+0100), Max Reitz wrote : >> On 05.03.2014 21:41, Benoît Canet wrote: >>> The Wednesday 05 Mar 2014 à 21:10:03 (+0100), Max Reitz wrote : >>>> On 05.03.2014 17:11, Benoît Canet wrote: >>>>> The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote : >>>>>> Implement this function in the same way as raw_bsd does: Acknowledge >>>>>> that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID >>>>>> and BDRV_BLOCK_DATA and derive the offset directly from the sector >>>>>> index) and add BDRV_BLOCK_RAW to the returned value. >>>>>> >>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>>>> --- >>>>>> block/json.c | 9 +++++++++ >>>>>> 1 file changed, 9 insertions(+) >>>>>> >>>>>> diff --git a/block/json.c b/block/json.c >>>>>> index a2f4691..7392802 100644 >>>>>> --- a/block/json.c >>>>>> +++ b/block/json.c >>>>>> @@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs, >>>>>> return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags); >>>>>> } >>>>>> +static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs, >>>>>> + int64_t sector_num, >>>>>> + int nb_sectors, int *pnum) >>>>>> +{ >>>>>> + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | >>>>>> + (sector_num << BDRV_SECTOR_BITS); >>>>>> +} >>>>> I don't understand what is the selling point of this method instead of calling >>>>> bdrv_co_get_block_status on bs->file. >>>>> Some information risk to be lost and it does look like magic. >>>> This is the same what "raw" does. It just is more meaningful: This >>>> way, this function does not pretend to provide the blocks itself but >>>> instead tells the truth; that is, the blocks are provided by an >>>> underlying BDS (bs->file). >>>> >>>> I wasn't really sure what to do myself. Generally, this driver is >>>> actually meant to pretend that it provides the blocks itself. On the >>>> other hand, I tried to imitate the behavior or "raw", since this is >>>> something I can hope to be approximately correct. Also, as I've said >>>> before, the value returned here is in fact at least technically >>>> correct. >>> The raw_bsd driver have an additional *pnum = nb_sectors; at the begining of the >>> function. Did you left it on purpose ? >> Oops, no, I did not. Okay, if I even fail at copying code, the >> argument about "raw" being at least probably correct isn't worth >> anything here, I guess. ;-) > In fact I wonder i posix_raw act this way because one of the BDRV it does wrap > have simply no clue on how to answer to this request. If you take a look at bdrv_co_get_block_status() in block.c, you'll see that BDRV_BLOCK_RAW actually results in a recursive call to bdrv_get_block_status(). Max > Best regards > > Benoît > >> Max >> >>> Best regards >>> >>> Benoît >>>> Max >>>> >>>>> Best regards >>>>> >>>>> Benoît >>>>> >>>>>> + >>>>>> static void json_invalidate_cache(BlockDriverState *bs) >>>>>> { >>>>>> return bdrv_invalidate_cache(bs->file); >>>>>> @@ -159,6 +167,7 @@ static BlockDriver bdrv_json = { >>>>>> .bdrv_aio_discard = json_aio_discard, >>>>>> .bdrv_co_write_zeroes = json_co_write_zeroes, >>>>>> + .bdrv_co_get_block_status = json_co_get_block_status, >>>>>> .bdrv_invalidate_cache = json_invalidate_cache, >>>>>> -- >>>>>> 1.9.0 >>>>>> >>>>>>
diff --git a/block/json.c b/block/json.c index a2f4691..7392802 100644 --- a/block/json.c +++ b/block/json.c @@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs, return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags); } +static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs, + int64_t sector_num, + int nb_sectors, int *pnum) +{ + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | + (sector_num << BDRV_SECTOR_BITS); +} + static void json_invalidate_cache(BlockDriverState *bs) { return bdrv_invalidate_cache(bs->file); @@ -159,6 +167,7 @@ static BlockDriver bdrv_json = { .bdrv_aio_discard = json_aio_discard, .bdrv_co_write_zeroes = json_co_write_zeroes, + .bdrv_co_get_block_status = json_co_get_block_status, .bdrv_invalidate_cache = json_invalidate_cache,
Implement this function in the same way as raw_bsd does: Acknowledge that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID and BDRV_BLOCK_DATA and derive the offset directly from the sector index) and add BDRV_BLOCK_RAW to the returned value. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/json.c | 9 +++++++++ 1 file changed, 9 insertions(+)