All of lore.kernel.org
 help / color / mirror / Atom feed
From: John Snow <jsnow@redhat.com>
To: Kashyap Chamarthy <kchamart@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	famz@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org,
	mnestratov@virtuozzo.com, armbru@redhat.com,
	nshirokovskiy@virtuozzo.com, stefanha@redhat.com,
	pbonzini@redhat.com, den@openvz.org, mreitz@redhat.com
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH for-2.12 0/4] qmp dirty bitmap API
Date: Thu, 7 Dec 2017 17:47:19 -0500	[thread overview]
Message-ID: <a172b331-77c7-f7bb-5bd1-2e59f0fdcde4@redhat.com> (raw)
In-Reply-To: <20171207115648.5mnvoyfnig7dhgbn@eukaryote>



On 12/07/2017 06:56 AM, Kashyap Chamarthy wrote:
> On Tue, Nov 21, 2017 at 07:10:19PM -0500, John Snow wrote:
>> On 11/21/2017 12:23 PM, Kevin Wolf wrote:
> 
> [...] # Snip lot of good discussion.
> 
>>> On another note, I would double check before adding a new block job type
>>> that this is the right way to go. We have recently noticed that many, if
>>> not all, of the existing block jobs (at least mirror, commit and backup)
>>> are so similar that they implement the same things multiple times and
>>> are just lacking different options and have different bugs. I'm
>>> seriously considering merging all of them into a single job type
>>> internally that just provides options that effectively turn it into one
>>> of the existing job types.
>>>
>>
>> I'm not particularly opposed. At the very, very least "backup" and
>> "mirror" are pretty much the same thing and "stream" and "commit" are
>> basically the same.
>>
>> Forcing the backuppy-job and the consolidatey-job together seems like an
>> ever-so-slightly harder case to make, but I suppose the truth of the
>> matter in all cases is that we're copying data from one node to another...
> 
> So from the above interesting discussion, it seems like Kevin is leaning
> towards a single job type that offers 'stream', 'commit', 'backup', and
> 'mirror' functionality as part of a single command / job type.  Based on
> an instinct, this sounds a bit too stuffy and complex to me.
> 
> And John seems to be leaning towards two block device job types:
> 
>   - 'blockdev-foo' that offers both current 'stream' and 'commit'
>     functionality as two different options to the same QMP command; and
> 
>   - 'blockdev-bar' will offer both 'mirror' and 'backup' functionality
>     as part of the same QMP command
> 
> FWIW, this seems a bit more palatable, as it is unifying
> similar-functionality-that-differ-slightly into two distinct commands.
> 
> (/me is still wrapping his head around the bitmaps and incremental
> backups infrastructure.)
> 

The discussion you're honing in on is tangential to the one at the heart
of this topic, which is:

"What's the right API for pull model incremental backups?" which digs up
the question "What is the right API for our existing data copying commands?"

Kevin is probably right in the end, though; he usually is. We do need to
unify our data moving logic to avoid fixing the same bugs in four
different but nearly identical jobs.

My only concern is that the special casing will grow too complex for
that one unified job and the job will become so complicated nobody knows
how to work on it anymore without breaking other cases. Maybe this is a
reasonable concern, maybe it isn't.

I'm also not in a hurry to personally unify the loops myself, but if
someone did, they could CC me and I'd review it thoroughly.

We'd find out in a hurry from the implementation if unification was a
win for SLOC.

--js

  parent reply	other threads:[~2017-12-07 22:47 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-13 16:20 [Qemu-devel] [PATCH for-2.12 0/4] qmp dirty bitmap API Vladimir Sementsov-Ogievskiy
2017-11-13 16:20 ` [Qemu-devel] [PATCH 1/4] block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap Vladimir Sementsov-Ogievskiy
2017-11-13 17:50   ` [Qemu-devel] [PATCH 1/4 for-2.11?] " Eric Blake
2017-11-16  7:56     ` Vladimir Sementsov-Ogievskiy
2017-11-16  7:59     ` Vladimir Sementsov-Ogievskiy
2017-11-13 16:20 ` [Qemu-devel] [PATCH 2/4] qapi: add block-dirty-bitmap-enable/disable Vladimir Sementsov-Ogievskiy
2017-11-13 16:20 ` [Qemu-devel] [PATCH 3/4] qmp: transaction support for block-dirty-bitmap-enable/disable Vladimir Sementsov-Ogievskiy
2017-11-13 16:20 ` [Qemu-devel] [PATCH 4/4] qapi: add block-dirty-bitmap-merge Vladimir Sementsov-Ogievskiy
2017-12-26  8:41   ` Vladimir Sementsov-Ogievskiy
2017-11-15 21:20 ` [Qemu-devel] [PATCH for-2.12 0/4] qmp dirty bitmap API John Snow
2017-11-16  8:17   ` Vladimir Sementsov-Ogievskiy
2017-11-17  3:10     ` John Snow
2017-11-17  8:22       ` Vladimir Sementsov-Ogievskiy
2017-11-17 21:35         ` John Snow
2017-11-21 17:23           ` Kevin Wolf
2017-11-22  0:10             ` John Snow
2017-11-22  8:40               ` Vladimir Sementsov-Ogievskiy
2017-12-07 11:56               ` [Qemu-devel] [Qemu-block] " Kashyap Chamarthy
2017-12-07 17:33                 ` Kevin Wolf
2017-12-08  9:35                   ` Kashyap Chamarthy
2017-12-07 22:47                 ` John Snow [this message]
2017-12-08 14:24                   ` Max Reitz
2017-11-30 12:10           ` [Qemu-devel] " Vladimir Sementsov-Ogievskiy
2017-12-07  0:38             ` John Snow
2017-12-07  9:39               ` Vladimir Sementsov-Ogievskiy
2017-12-09  0:57                 ` John Snow
2017-12-11  9:14                   ` Denis V. Lunev
2017-12-11 11:15                   ` Kevin Wolf
2017-12-11 12:18                     ` Vladimir Sementsov-Ogievskiy
2017-12-12 22:15                       ` John Snow
2017-12-11 18:40                     ` John Snow
2017-12-12 11:16                       ` Kevin Wolf
2017-11-20 16:00       ` Denis V. Lunev
2017-11-24 15:01         ` Vladimir Sementsov-Ogievskiy
2017-11-27 12:04           ` Kevin Wolf
2017-12-13  4:12 ` Fam Zheng
2017-12-19 16:07   ` Vladimir Sementsov-Ogievskiy
2017-12-20  1:06     ` John Snow
2017-12-20  8:05       ` Nikolay Shirokovskiy
2017-12-20  8:20       ` Vladimir Sementsov-Ogievskiy
2017-12-20 10:29         ` Kirill Korotaev
2017-12-26  7:07         ` Fam Zheng
2017-12-26  8:57           ` Vladimir Sementsov-Ogievskiy
2017-12-26  9:45             ` Fam Zheng

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a172b331-77c7-f7bb-5bd1-2e59f0fdcde4@redhat.com \
    --to=jsnow@redhat.com \
    --cc=armbru@redhat.com \
    --cc=den@openvz.org \
    --cc=famz@redhat.com \
    --cc=kchamart@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mnestratov@virtuozzo.com \
    --cc=mreitz@redhat.com \
    --cc=nshirokovskiy@virtuozzo.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    --cc=vsementsov@virtuozzo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.