Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

["Denis V. Lunev" <den@openvz.org>]

On 11/17/2015 10:22 AM, Stefan Hajnoczi wrote:
> On Mon, Nov 16, 2015 at 06:24:36PM +0300, Denis V. Lunev wrote:
>> +int bdrv_all_find_snapshot(const char *name, bool skip_read_only,
>> +                           BlockDriverState **first_bad_bs)
>> +{
>> +    QEMUSnapshotInfo sn;
>> +    int err = 0;
>> +    BlockDriverState *bs = NULL;
>> +
>> +    while (err == 0 && (bs = bdrv_next(bs))) {
>> +        AioContext *ctx = bdrv_get_aio_context(bs);
>> +
>> +        if (skip_read_only &&
>> +                (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs))) {
> These must be called with AioContext acquired.
>

AFAIK no. this is called without that in bdrv jobs code.

bdrv_is_read_only is a simple flag, which is filled on open
bdrv_is_inserted is defined for CDROM only and does
not rely on AIO context stuff.

>> +            continue;
>> +        }
>> +
>> +        aio_context_acquire(ctx);
>> +        if (bdrv_can_snapshot(bs)) {
> The !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) checks above are
> redundant since bdrv_can_snapshot() checks too:
>
>    int bdrv_can_snapshot(BlockDriverState *bs)
>    {
>        BlockDriver *drv = bs->drv;
>        if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
>            return 0;
>        }
>
> It also means that the "skip_read_only" name is inaccurate.  Read-only
> drives are always skipped, regardless of skip_read_only's value.
>
> The skip_read_only argument can be dropped and the earlier
> !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) check can be dropped too.
I have though on this several times and once again I am wrong :(
This looks like a punishment for my sins. OK :(


>> @@ -2168,21 +2157,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
>>       available_snapshots = g_new0(int, nb_sns);
>>       total = 0;
>>       for (i = 0; i < nb_sns; i++) {
>> -        sn = &sn_tab[i];
>> -        available = 1;
>> -        bs1 = NULL;
>> -
>> -        while ((bs1 = bdrv_next(bs1))) {
>> -            if (bdrv_can_snapshot(bs1) && bs1 != bs) {
>> -                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
>> -                if (ret < 0) {
>> -                    available = 0;
>> -                    break;
>> -                }
>> -            }
>> -        }
>> -
>> -        if (available) {
>> +        if (bdrv_all_find_snapshot(sn_tab[i].id_str, false, &bs1) == 0) {
> bdrv_all_find_snapshot() doesn't do the bs1 != bs exclusion so the new
> code behaves differently from the old code.  That seems like a bug.
no. The result will be the same. This is a minor optimisation:
- we get the first block device we can make snapshot on
- we get snapshot list on that device
- after that we start iterations over the list and removing not 
available snapshots on other devices
This means that if bs == bs1 the check will be "always true"