rationale for a PGLog::merge_old_entry case

rationale for a PGLog::merge_old_entry case

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

Hi Sam,

TL;DR:

When there no new entry, what is the rationale for merge_old_entry to remove the object from missing only if the tail is eversion_t() and the object prior_version is also eversion_t() ?
https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/osd/PGLog.cc#L330

Long version:

The conditions are created with:

    info.log_tail = eversion_t();
    oe.soid.hash = 1;
    oe.op = pg_log_entry_t::DELETE;
    oe.prior_version = eversion_t();
    
    missing.add(oe.soid, eversion_t(1,1), eversion_t());

as shown in
https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/test/osd/TestPGLog.cc#L467
I double checked with gdb and when called with

EXPECT_FALSE(merge_old_entry(t, oe, info, remove_snap, dirty_log));
https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/test/osd/TestPGLog.cc#L481

it reaches

missing.rm(oe.soid, missing.missing[oe.soid].need);
https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/osd/PGLog.cc#L330

and the expected side effects are observed:

    EXPECT_FALSE(dirty_log);
    EXPECT_TRUE(remove_snap.empty());
    EXPECT_TRUE(t.empty());
    EXPECT_FALSE(missing.have_missing());
    EXPECT_TRUE(log.empty());
    EXPECT_EQ(0U, ondisklog.length());
https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/test/osd/TestPGLog.cc#L483

Cheers

-- 
Loïc Dachary, Artisan Logiciel Libre
All that is necessary for the triumph of evil is that good people do nothing.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: rationale for a PGLog::merge_old_entry case

[Samuel Just]

In all three cases, we know the authoritative log does not contain an
entry for oe.soid, therefore:

If oe.prior_version > log.tail, we must already have processed an
earlier entry for that object resulting in the object being correctly
marked missing (or not) (specifically, the entry for
oe.prior_version).

If log.tail >= oe.prior_version > eversion_t(), the missing entry
should have need set at oe.prior_version (revise_need).
oe.prior_version cannot be divergent because all divergent entries
must fall within the log (otherwise, we would have backfilled).

If oe.prior_version == eversion_t(), the object no longer exists, and
the object should be removed from the missing set.

Hope that helps.
-Sam

On Sun, Jun 2, 2013 at 4:09 AM, Loic Dachary <loic@dachary.org> wrote:
> Hi Sam,
>
> TL;DR:
>
> When there no new entry, what is the rationale for merge_old_entry to remove the object from missing only if the tail is eversion_t() and the object prior_version is also eversion_t() ?
> https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/osd/PGLog.cc#L330
>
> Long version:
>
> The conditions are created with:
>
>     info.log_tail = eversion_t();
>     oe.soid.hash = 1;
>     oe.op = pg_log_entry_t::DELETE;
>     oe.prior_version = eversion_t();
>
>     missing.add(oe.soid, eversion_t(1,1), eversion_t());
>
> as shown in
> https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/test/osd/TestPGLog.cc#L467
> I double checked with gdb and when called with
>
> EXPECT_FALSE(merge_old_entry(t, oe, info, remove_snap, dirty_log));
> https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/test/osd/TestPGLog.cc#L481
>
> it reaches
>
> missing.rm(oe.soid, missing.missing[oe.soid].need);
> https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/osd/PGLog.cc#L330
>
> and the expected side effects are observed:
>
>     EXPECT_FALSE(dirty_log);
>     EXPECT_TRUE(remove_snap.empty());
>     EXPECT_TRUE(t.empty());
>     EXPECT_FALSE(missing.have_missing());
>     EXPECT_TRUE(log.empty());
>     EXPECT_EQ(0U, ondisklog.length());
> https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/test/osd/TestPGLog.cc#L483
>
> Cheers
>
> --
> Loïc Dachary, Artisan Logiciel Libre
> All that is necessary for the triumph of evil is that good people do nothing.
>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rationale for a PGLog::merge_old_entry case

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 2889 bytes --]

Hi Sam,

Thanks for the explanation. I misread the third case, my description was incorrect. I amended https://github.com/ceph/ceph/pull/340 accordingly.

Cheers

On 06/03/2013 10:28 PM, Samuel Just wrote:
> In all three cases, we know the authoritative log does not contain an
> entry for oe.soid, therefore:
> 
> If oe.prior_version > log.tail, we must already have processed an
> earlier entry for that object resulting in the object being correctly
> marked missing (or not) (specifically, the entry for
> oe.prior_version).
> 
> If log.tail >= oe.prior_version > eversion_t(), the missing entry
> should have need set at oe.prior_version (revise_need).
> oe.prior_version cannot be divergent because all divergent entries
> must fall within the log (otherwise, we would have backfilled).
> 
> If oe.prior_version == eversion_t(), the object no longer exists, and
> the object should be removed from the missing set.
> 
> Hope that helps.
> -Sam
> 
> On Sun, Jun 2, 2013 at 4:09 AM, Loic Dachary <loic@dachary.org> wrote:
>> Hi Sam,
>>
>> TL;DR:
>>
>> When there no new entry, what is the rationale for merge_old_entry to remove the object from missing only if the tail is eversion_t() and the object prior_version is also eversion_t() ?
>> https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/osd/PGLog.cc#L330
>>
>> Long version:
>>
>> The conditions are created with:
>>
>>     info.log_tail = eversion_t();
>>     oe.soid.hash = 1;
>>     oe.op = pg_log_entry_t::DELETE;
>>     oe.prior_version = eversion_t();
>>
>>     missing.add(oe.soid, eversion_t(1,1), eversion_t());
>>
>> as shown in
>> https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/test/osd/TestPGLog.cc#L467
>> I double checked with gdb and when called with
>>
>> EXPECT_FALSE(merge_old_entry(t, oe, info, remove_snap, dirty_log));
>> https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/test/osd/TestPGLog.cc#L481
>>
>> it reaches
>>
>> missing.rm(oe.soid, missing.missing[oe.soid].need);
>> https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/osd/PGLog.cc#L330
>>
>> and the expected side effects are observed:
>>
>>     EXPECT_FALSE(dirty_log);
>>     EXPECT_TRUE(remove_snap.empty());
>>     EXPECT_TRUE(t.empty());
>>     EXPECT_FALSE(missing.have_missing());
>>     EXPECT_TRUE(log.empty());
>>     EXPECT_EQ(0U, ondisklog.length());
>> https://github.com/dachary/ceph/blob/f58299db098d5f18c817b516fa6ffaa76245e57d/src/test/osd/TestPGLog.cc#L483
>>
>> Cheers
>>
>> --
>> Loïc Dachary, Artisan Logiciel Libre
>> All that is necessary for the triumph of evil is that good people do nothing.
>>

-- 
Loïc Dachary, Artisan Logiciel Libre
All that is necessary for the triumph of evil is that good people do nothing.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]