pmemobj_tx_xadd_range ignores flags when merging ranges

[teshull]

ISSUE:

pmemobj_tx_xadd_range ignores flags when merging ranges

Environment Information

• PMDK package version(s): 1.6
• OS(es) version(s): any
• ndctl version(s): any
• kernel version(s): any

Please provide a reproduction of the bug:

Slightly modified version of do_tx_xadd_range_commit(PMEMobjpool *pop) in the obj_tx_add_range.c test

static void
do_tx_xadd_range_commit_error(PMEMobjpool *pop)
{
    int ret;
    TOID(struct object) obj;
    TOID_ASSIGN(obj, do_tx_zalloc(pop, TYPE_OBJ));

    TX_BEGIN(pop) {
        ret = pmemobj_tx_xadd_range(obj.oid, VALUE_OFF, VALUE_SIZE,
                POBJ_XADD_NO_FLUSH);
        UT_ASSERTeq(ret, 0);
        ret = pmemobj_tx_xadd_range(obj.oid, VALUE_OFF, VALUE_SIZE,
                0);
        UT_ASSERTeq(ret, 0);

        D_RW(obj)->value = TEST_VALUE_1;
        /* let pmemcheck find we didn't flush it */
    } TX_ONABORT {
        UT_ASSERT(0);
    } TX_END

    UT_ASSERTeq(D_RO(obj)->value, TEST_VALUE_1);
}

This will not flush the field to memory at the end of the transaction. I validated the behavior using pmemcheck.

How often bug is revealed: (always, often, rare):

always

Actual behavior:

The flag associated with the first call to tx_xadd_range(_direct) for merged ranges is always used.

Expected behavior:

While not explicitly covered within the manual, the most intuitive behavior is to have
the flag for a range representing multiple merged calls to tx_xadd_range(_direct) to be merged to its most conservative state.

Details

Currently in pmemobj_tx_add_common when a new range can be merged into an existing range already present within the range AVL tree, the new range's flags are ignored.

This is not intuitive behavior. Reasonable alternatives include:

1. Merge the ranges, but set the range's flag to be the most conservative of the ranges merged (i.e. POBJ_XADD_NO_FLUSH is only set if it is set within all merged ranges)
2. Divide the ranges into multiple ranges based on the flags associated with each range

Additional information about Priority and Help Requested:

Are you willing to submit a pull request with a proposed change? Yes

Requested priority: Low

[marcinslusarz]

Indeed, this can be a problem for some applications. Fixing this may be challenging though because the tx_[x]add_range[_direct] implementation is quite complex and this new logic must be enabled only for the NO_FLUSH flag, making the implementation even more complex. (On master, we have a new flag which should not trigger this.)

If you are going to fix this, please submit the PR against the stable-1.5 branch. From there the fix will be applied automatically to newer branches.

[teshull]

To me, both NO_FLUSH and the new NO_SNAPSHOT flag seem more like performance optimization hints rather than behaviors which must be followed. Otherwise, what is the desired behavior when there are two tx_xadd_ranges[_direct] to the same address, one with NO_SNAPSHOT, and the other without it?

[marcinslusarz]

NO_SNAPSHOT means "this buffer is not initialized, so do not snapshot it, but I may store to it, so please track it as part of the transaction (e.g. tell Valgrind pmemcheck about it) and flush it on commit". Next tx_add of the same region without NO_SNAPSHOT doesn't have to snapshot it.

[teshull]

Okay, your explanation makes sense.

With regards to merging the NO_FLUSH flag, which of the approaches I mentioned above do you think is appropriate? I believe the most reasonable approach is my first suggestion of still merging the ranges, but setting the range's new flag to be the most conservative of the merged ranges. This would not require a lot of changes to the code, and I feel this scenario shouldn't happen often enough to merit the more intrusive changes needed to split (and maybe re-merge) ranges based on the flag state.

[marcinslusarz]

The more I look at pmemobj_tx_add_common the more I don't like it. Fixing it properly (option 2) would probably mean rewriting this function, so I'm not sure I would like to see such a bug fix in a stable version.

So the best course of action would be to make a quick fix (option 1, basically clear no_flush flag in the existing region if new snapshot doesn't have it) with tests, merge it to stable-1.5, stable-1.6 and master and then fix it properly only on master.

What do you think?

[teshull]

Yes, this sounds reasonable to me. I will submit a patch for the quick fix and test cases in the near future.

[marcinslusarz]

Any progress? We are rapidly approaching a new major release and I'd like to have this fix in.

[teshull]

Sorry for the delay - got caught up in other work. I'll submit a patch by Wednesday.

[teshull]

I added a pull request with a fix (pull request 3924)