[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[dm-devel] Re: Shared snapshots




On May 6, 2009, at 11:06 AM, Mikulas Patocka wrote:

Hi

I'd like to state that the point of the refactoring work was not to reduce the size of the code. The point was to provide a clear and clean interface for all exception stores - new and old - and do it in a way that adheres to the
principals of the general device-mapper target API.

First let me write about a possible arguments regarding "cleanness",
"niceness" etc.

I think that what is really hapenning here is loss of reason.

In general, loss of reason is a cognitive bias that happens in the
following scenario (not only in software engineering, but in other social
situations as well):

* people want to reach goal G
* in order to reach the goal G, people do action A
* they do it thoroughly, consistently, over the long time
* now, "loss of reason", an error inside human brain happens: people start slowly forgetting about the goal G. And they start viewing the action A as
being "good", "nice", "right", "the way it should be done"
* from this point on, people are doing action A even in situations when
it damages the goal G
* people's performance decreases, they no longer attempt to reach real
objective goals, all they want to reach is their own emotion of "niceness"
inside their head.

Now, how does it apply to our snapshot development scenario?

* programmers want to reduce coding time and maintenance time (goal G)
* to reach this goal, programmers share their code, they sometimes
refactor code to be better shareable (action A)
* it has been done consistently over the long time for various projects
* now people start considering sharing code and refactoring as "good",
"clean", "the way software should be written" or whatever. They completely
forgot about the initial goal that was there (save developers' time)
* and now we are refactoring the snapshot code even if increases our
development time, it is not going to save the time in the future, but we
consider this "right", "clean", "clear", "nice"

Are you getting it how these "universal"/"emotional"/"aesthetic" arguments
not backed by anything are void?


Now, if we ignore the emotional stuff and look at it from the real point
of view.

Is the refactored code saving development time now?
- it isn't. It already ate 5 months and it will just eat more before it
gets functional.

I agree. I think there will be additional time involved to get the refactored code ready. I also believe that it will take additional time to get your current solution ready. There is also more involved here than just development time. You must also consider the time it takes to merge things upstream. If you are unwilling to make any concessions on your code, it is unlikely to go upstream. Where will you be then?

Will the refactored code be saving maintenance time in the future?
- it won't. The code is as big as if the refactoring hadn't happened (and
it'll be just bigger if you imeplement missing functionality).

Anyway, two smaller drivers are better maintainable than one big driver of
the double size. A person who doesn't understand the code will have to
read just half of the code to understand the logic if he wants to modify one of the smaller drivers. It also reduces testing time, if you modify one smaller driver, you are certain that the second driver is intact and
don't have to test it.

I see (have seen) your point; and while I respectfully disagree, I still think I have ideas where we can meet in the middle to come up with a solution. (More on this later.)

Is your refactored code extensible to other exception stores?
- true. It is. But it was extensible even before refactoring. Before
refactoring, one could write any exception store driver (either unshared
or shared) and plug it into the unshared-master-driver or the
shared-master-driver. All you did was to join the unshared-master- driver
and the shared-master-driver to one chunk of code.

You make it sound as though there was a modular interface to begin with, but there was not. Every time a new exception store was added in the past, it required new parsing code in the snapshot module, new 'if' statements to conditionalize for it, etc. A bunch of patches have already been added to improve that situation - all of which were part of the refactored code. If just 2 of the remaining 33 patches in my further refactoring are added upstream, then there is no need for the snapshot module to parse the exception store arguments. In fact, there is no reason for the snapshot module to know anything about how the exception store does its job.

Any other advantages?

Yes. It is easier to write/stack/include cluster-aware exception store modules.

Regarding the bugs:
56MB origin, 160MB snapshot store (128MB of that are reserved for
non-existing journal, so it will overflow). Run mkfs.reiserfs on the
origin. Ends up with a lot of messages like this:
May 6 13:36:10 slunicko kernel: attempt to access beyond end of device May 6 13:36:10 slunicko kernel: dm-5: rw=25, want=1048784, limit=327680 May 6 13:36:11 slunicko kernel: attempt to access beyond end of device May 6 13:36:11 slunicko kernel: dm-5: rw=25, want=1376256, limit=327680 May 6 13:36:11 slunicko kernel: attempt to access beyond end of device
and the process stucks in "D" state in
__wait_on_bit/wait_on_page_writeback_range. Vely likely the bio was lost
somewhere and never returned with either error or success.

I have found a couple of bugs in Fujita/Daniel's exception store long
time ago (when I was porting it to my driver), the most serious one I
found was this:
@@ -510,7 +510,7 @@ static int read_new_bitmap_chunk(struct
       chunk_io(store, ss->cur_bitmap_chunk, WRITE, 1, ss->bitmap);

       ss->cur_bitmap_chunk++;
-       if (ss->cur_bitmap_chunk == ss->nr_bitmap_chunks)
+ if (ss->cur_bitmap_chunk == ss->nr_bitmap_chunks + FIRST_BITMAP_CHUNK)
               ss->cur_bitmap_chunk = FIRST_BITMAP_CHUNK;

       chunk_io(store, ss->cur_bitmap_chunk, READ, 1,  ss->bitmap);
... but it doesn't fix this problem. So there's something else.

Maybe the bad accesses could be due to endianity problems, I use sparc64. The lost bio can't be caused by endianity, this needs to be investigated.


Writable snapshots:
create an origin and two snapshots. Write to the origin (it creates shared
chunk for both snapshots). Write to one of the snapshots to the same
chunk. The write is lost. Read it again and the data is not there.

In this situation you need to copy the chunk from the snapshot store to the snapshot store (you can't copy it from the origin). I was looking for the code that handles these snapshot-to-snapshot copies, but I didn't find
it. So the problem is not some silly typo that could be fixed with one
line - the problem is that the code to handle it wasn't written.

Thank-you for explaining these bugs. You are right, the code to handle snapshot-to-snapshot copy does not exist! It should not be difficult to fix these bugs.

I tried to ensure that all necessary API functions were in place. Like "suspend", which is not used by any of the legacy exception stores, but will be useful when it comes time to quiesce a shared exception store that may be
performing a "delete" operation.

You can't do this. "delete" is a long-running operation, it needs to walk
the whole tree. Stopping an exception store during delete is not
acceptable on big devices. I have already implemented delete in the
background, it goes step-by-step and blocks concurrent access only for
these little steps, so it won't lock the computer up for minutes or hours.
It can also batch multiple delete requests just into one tree-walk.

Your approach is excellent. However, it would be unacceptable if it cannot cease processing when device-mapper needs it to. Certainly you built in the capability to stop and restart the process? I am not asking for the delete operation to be completed in order to suspend. I am asking for the operation to halt for a suspend and continue on resume. This is a fundamental requirement. How will you know when to suspend if you are not informed by a 'suspend' call from the API?

Please note, that there probably won't be that much code sharing between exception store modules; but the snapshot code is completely agnostic to the type of exception store used. This allows an infinite variety of exception stores without change to the snapshot code. This is also nice because changes in the snapshot code should not require work in the exception stores and vise versa. Userspace code should also be reduced due to identical mechanisms used to communicate and operate snapshots with legacy or future exception stores - you need only change the constructor arguments. Backwards compatibility is improved and the meaning of DM ioctls doesn't change depending on the target type. It is a necessity that we have new exception stores, but I don't think that necessitates a new snapshot target - something which, I think, would increase our maintenance costs because we now support two entirely different
snapshot implementations.

We absolutely need your exception store - there is no question about that. I

But it is not possible now. What it lacks:
- the snapshot ids are submitted by the user (in my implementation, the exception store itself must make the snapshot id, it is design thing that
can't be changed)

Really? It can't be changed? I think it would be great if you added a translation table. Userspace would pass down a number/name/UUID (it doesn't really matter, as long as it is unique among the shared snapshots for a given COW device), then your exception store could make its own "snapshot id" and associate it with what userspace has given it.

This translation table could clean up a number of complaints that we have had about your implementation. Now, all you should need is a simple CTR + RESUME to get a snapshot device going - same as the legacy snapshot targets and every other device-mapper target without exception. There would no longer be a need to use 'message' to create a snapshot and then a further CTR+RESUME to instantiate it.

Additional benefits could be gained from this. I no longer see a need to have the origin constructor changed... Now things seem to be coming more in-line with the way the userspace/kernel interface was in the past. That should simplify userspace coding.

Just this one thing could fix so many complaints... can you not do this?

- it lacks read-once-write-many copies (they are needed only on my store,
not on Fujita-Daniel's store)

I need to understand this better. Perhaps you could help me? This could be your greatest argument. It would not deter me from asking for a exception store/snapshot boundary API, nor will it deter me from demanding better userspace/kernel boundary compliance. However, it may make me realize that retrofitting the old snapshot module to also be able to handle shared exception stores is not the best. (This would require me to remove 2 of the 33 outstanding patches I have posted.)

If you help me, I will probably understand this code more quickly. Otherwise, it will take me longer. If I can see no solution, I will very easily accept that the current snapshot module is not the place to handle shared exception stores. I have to warn you though... I can be good at finding solutions to problems.

- it lacks snapshot-to-snapshot copies for writable snapshots (for
Fujita-Daniel's code you need just one copy, for my code you need at most
two).

I think there are solutions to this that are not that complicated.

- it lacks the possibility to re-submit bios to the exception store to do
more relocations after some of the relocations have been finished. (in
extreme cases of many writes to the snapshots, there may be many
relocations on my exception store --- it is no need to optimize for these
cases, but they must be handled correctly).

I also would like to understand this limitation more.

- lame delete that blocks the whole device.

What? There is no reason to block the whole device for a delete - that is an exception store implementation detail that has nothing to do with the snapshot module or the exception store API.

agree with your complaints about the short-comings of the shared exception store that was ported to the interface (e.g. it has no journaling, 64 share limit before needing a new store, memory issues). I ported the shared
exception store to the interface (along with creating a cluster-aware
exception store) so I could feel confident that I got the interface correct. The "shared" exception store should not be viewed as complete; even though I think the exception store interface is. Please refrain from judging the API
based on incompleteness in the implementation of one exception store.

This is not about lameness of Fujita's code. What I mean is that your
interface is too tied on Fujita-Daniel's store. It is not just easy that you take my store and plug it in. It still needs many generic changes. It is not as beautiful as you described it that it is "completely agnostic to the type of exception store used". And I am concerned --- why spend more
time making the changes, if generic driver that has them is already
written?

Now trying to aggregate comments... see below.



[However, I have tested the "shared" exception store and did not suffer from the same problems you did. I would be interested to have you fill me in on the problems you encountered - "doesn't support writable snapshots correctly, it deadlocks when the exception store overflows, it prints warnings about circular locking"]. I believe I have the interface correct, and I am not aware of changes that would have to be made to accommodate your exception
store.

I also think you have a number of great ideas in your higher level snapshot code. I'd love to see these added to the current snapshot module. I would even be willing to abandon the current snapshot module and make yours the only
one in the kernel iff it adhered to the following principals:

1) It is completely agnostic to all legacy and future exception stores

My code does support all existing and possible future shared
snapshots. It doesn't support non-shared snapshots. They are well
implemented already and there is no reason to reinvent the wheel.

Now trying to aggregate comments... see below.



(e.g.
it should know nothing about snapid's and such, as this information aids in how/where an exception store places data - something that should be known only to the exception store implementation) Without a clean break, the API is
meaningless.

My code works in such a way that the kernel creates a snapid, it passes it to the userspace, the userspace is supposed to store the snapid in its lvm metadata and passes it back to the kernel. The userspace views the snapid as an opaque value --- it doesn't look at the snapid, it just passes it
back and forth.

If you wanted to change it to uuids in the kernel, it is changeable.

Now trying to aggregate comments... see below.



2) The current meaning of DM ioctls is preserved. We shouldn't have to create/resume a snapshot device and then use messages or other mechanisms to instantiate the shares. This slippery slope leads us to a place where each target gets to choose its userspace interaction and the defined functions for
that purpose become meaningless.

It is changeable to uuids if the kernel maintained uuid->snapid
translation table in the exception store. It is possible, thought we must
know why. "snapids are submitted by userspace" is not possible.

Ultimately, I think the exception store API that I have put in place would be the glue between the upper snapshot layer and the lower exception store modules - whatever generation of these layers happens to exist. I believe the
API follows in the models laid down by the dm-raid1/log and
multipath/read-balance interfaces.  I think this API is a natural and
necessary progression.

I would also take issue with the fact that if the code size is increased, it decreases maintainability. I think maintainability is improved with the clean separation of snapshot code and exception store modules; just as I think maintainability is improved by having a file system switch that separates the different file systems and provides a clean method for them to communicate
with higher layers.

I welcome further thoughts,
brassow

Why don't do this refactoring and why not have common driver for shared and non-shared exception store? - it already took 5 months, it is still incomplete and needs non-trivial changes to be at least as functional as
if this hadn't happened at all.

If you're still thinking about "cleanness", read the first paragraph about
loss of reason again. Back into reality --- if you spend 5 months
shuffling code around without adding any functionality it is only
reasonable to do it if it will save more than 5 months in the long term.
And it won't.

It is no shame, it occasionaly happens to anyone. To me it happened on
other projects too that I had these feelings "wow, it will be super clean and easy" and when I got into typing the code, it turned out that it is much more time-consuming that I anticipated. And when it happend to me, I erased the code and changed my perception of "cleanness" - it's the only
thing one can do.

Final comments...

You and I are in agreement that we don't want to waste time; and certainly, there is no time to waste. We want a good solution. Let us figure out what we both want; and determine from there what pieces of code we can keep and what needs to be adjusted. I will start...

Here is what I demand:
#1) A clean API that separates the snapshot module and the exception store implementations. This facilitates others who wish to write new exception stores - including me, who is responsible for writing the cluster-aware exception store. I would appreciate it if some forethought was given to cluster implementations. I am not saying that there cannot be a "shared" API and a "non-shared" API. Go ahead, write your own - it shouldn't take too long. I'd like to think that there can be one API that can satisfy the needs of both (perhaps even with a flag that signifies "shared" vs "non-shared if necessary?). I will continue to think that it is possible to make this happen until I am proved or can prove to myself otherwise. This does /not/ mean I will block any attempts to do something other than a singular, all- inclusive API.

#2) Don't break the current model of userspace/kernel interaction unless it is absolutely necessary. 'delete'ing a single snapshot from a share exception store is *potentially* one of these cases (although I can think of good solutions). Right now, when we want to delete a snapshot, we simply wipe the COW device - obviously, this doesn't work when the COW is shared. However, forcing userspace to know about snapid's which are generated by the kernel and overloading the message function is not acceptable. I think there is also much room for improvement once that is fixed to minimize changes to the constructor tables and reduce the burden on user-space tools.

Here is what I am willing to sacrifice if it proves necessary:
#1) I am willing to give in on having two snapshot modules. I have always stated this as a possibility. It is very possible that it makes more sense this way instead of having one snapshot module be able to interact with all exception stores. This is even more palatable if you consider that the legacy snapshots will be largely abandoned. This would mean abandoning some of the patches I've posted upstream, but others still remain quite useful. More than one snapshot-merge target might be required in this case. There are a number of other disadvantages to this that I (maybe not others) would be willing to set aside based solely on your word that the advantages outweigh the disadvantages.

#2) I am willing to put in any extra work required to write the cluster-aware modules - even if two APIs and two snapshot modules end up being required.

Mikulas, I get the sense that you have looked at the API with a quick mind of why it can't be done, instead of whether it can be done. The conclusions may ultimately be the same; but I am asking you to take a look and see what *can* be done. What compromises can be made? How far are you willing to come? I don't think my base demands are too much to ask, nor do I think they materially differ from what is required for upstream inclusion.

Thanks for your continued effort on this,
 brassow


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]