Bitmap optimization with experimental and debug options
Jiaying Zhang
I tried your patch but the two added options don't seem to work.
I saw you set the superblock flags in ddsnapd.c:start_server().
But those flags will be overwritten in load_sb() that is called when
the 'ddsnap server' receives START_SERVER message. Can we
just use global variables instead?
Jiaying
Daniel Phillips
> Hi Daniel,
>
> I tried your patch but the two added options don't seem to work.
> I saw you set the superblock flags in ddsnapd.c:start_server().
> But those flags will be overwritten in load_sb() that is called when
> the 'ddsnap server' receives START_SERVER message. Can we
> just use global variables instead?
We should fix the flaw in load_sb, instead of going to global variables
which would all have to be undone if we move the server to kernel.
I will provide a fix and test it.
Regards,
Daniel
Daniel Phillips
> Hi Daniel,
>
> I tried your patch but the two added options don't seem to work.
> I saw you set the superblock flags in ddsnapd.c:start_server().
> But those flags will be overwritten in load_sb() that is called when
> the 'ddsnap server' receives START_SERVER message. Can we
> just use global variables instead?
OK, I'm staying with my "don't hack it, fix it" stance on this one.
When I looked at the code in question I found the entire load_sb
function was bogus, called in only one place, and zeroing variables
it should not be zeroing (they are already zeroed in new_sb).
I moved the load_sb code into the START_SERVER case in the main
loop, where some related code already existed.
There are actually two flags fields in the superblock, one for
the image (64 bits) and one for runtime. This is intensely
confusing and in fact I found a bug there: the SB_DIRTY flag was
stored on disk, which clearly makes no sense. So I renamed the
flags field that is not stored on disk as "runflags" to clarify
things. I removed SB_DIRTY from the sbflags enumeration, and
created a new enumeration for runtime flags not stored on disk.
I changed SB_DIRTY to RUN_SB_DIRTY to clarify the distinction
(probably getting a bit verbose there but oh well, the bug I just
noticed won't happen again).
I was also erroneously storing the SB_DEFER flag in the disk
superblock, which is actually what caused the bug you saw, not the
erroneous initialization in load_sb. Gah! The things you find
when you read mature code closely.
May I get up on a pulpit for a moment? It is nearly always better
to do a proper fix than a quick hack just to make something work.
In this case I found that the apparent bug was not the real bug.
It often takes a little longer to do a proper fix but it is said
that in the long run, it actually saves time.
Not tested! I will ping the list when it is tested.
Daniel
Jiaying Zhang
I tried your patch and it still doesn't set those flags. I think you should
assign the flags to sb->runflags instead of sb->image.flags in start_server().
I also found that check_freespace complains about wrong freechunk count
while testing the patch. I found you removed the commit_transactions(sb, 0)
I added in commit_deferred_allocs(). As I explained in the email I send
to you a couple of weeks ago:
"The commit_deferred_allocs() function writes all of chunks recorded
in the deferred_alloc to disk. It then sets sb->deferred_allocs to zero
and then calls commit_transaction with barrier parameter set to 1. The
problem is that in this case, sb->defer.count can be non-zero. So the
commit_deferred_allocs() function does not flush all of the deferred
allocated chunks to disk."
You will see the problem with the ddsnap_replay_journal.sh cbtb test
if you have check_freespace turned on. I am going to modify that test
with the added "-X" and "-D" flags once your patch is committed.
The modified patch is attached. Here is the diff based on your patch.
Except the two changes mentioned above, the event_hook change is
for testing replay_journal and has been committed last week.
--- ddsnapd.c 2008-05-16 14:41:36.000000000 -0700
+++ /root/zumastor-test/ddsnap/ddsnapd.c 2008-05-16 14:43:16.000000000 -0700
@@ -670,6 +670,7 @@ static void commit_transaction(struct su
static void commit_deferred_allocs(struct superblock *sb)
{
+ commit_transaction(sb, 0);
flush_deferred_allocs(sb);
commit_transaction(sb, 1);
}
@@ -4119,6 +4120,7 @@ static int incoming(struct superblock *s
static int cleanup(struct superblock *sb)
{
+ event_hook(0, SHUTDOWN_SERVER); /* event hook for abort action */
commit_deferred_allocs(sb);
sb->image.flags &= ~SB_BUSY;
set_sb_dirty(sb);
@@ -4448,7 +4450,7 @@ int start_server(
if (!valid_sb(sb))
error("Invalid superblock: please run 'ddsnap-sb' first to upgrade the superblock.\n");
- sb->image.flags = flags;
+ sb->runflags = flags;
unsigned bufsize = 1 << sb->image.metadata.allocsize_bits;
if (cachesize_bytes == 0) {
Jiaying
Daniel Phillips
I'm slowing everybody down by not checking in this patch. Let's just
do it side by side today, ok? Here is the latest version posted to our
list. See you later.
Daniel
Jiaying Zhang
I just found I forgot to put another change in my last email.
There should be a brackets surrounding the flag and.
--- /root/ddsnap-tmp/ddsnapd.c 2008-05-19 14:44:25.000000000 -0700
+++ ddsnapd.c 2008-05-19 15:13:43.000000000 -0700
@@ -335,7 +335,7 @@ static inline unsigned chunk_sectors(str
static int deferring(struct superblock *sb)
{
- return !!sb->runflags & RUN_DEFER;
+ return !!(sb->runflags & RUN_DEFER);
}
/*
Jiaying