prepare_write count check

[biovoid]

Hi,

In the function prepare_write() the expression m->count >= dhara_map_capacity(m) stands before checking trace_path(m, dst, NULL, meta, &my_err). And in the case when map count is equal to map capacity returns error DHARA_E_MAP_FULL. But if page is exists in trace then count should not be incremented and the check is wrong. If I'm understand it right then the check should be only before incrementing m->count++;

[dlbeer]

On Tue, Apr 14, 2020 at 10:35:52AM -0700, Sergey Kashlikov wrote:

In the function prepare_write() the expression m->count >= dhara_map_capacity(m) stands before checking trace_path(m, dst, NULL, meta, &my_err). And in the case when map count is equal to map capacity returns error DHARA_E_MAP_FULL. But if page is exists in trace then count should not be incremented and the check is wrong. If I'm understand it right then the check should be only before incrementing m->count++;

Hi Sergey,

I'm not sure I understand. The capacity check is performed before incrementing m->count, and m->count not incremented if the page already exists.

If you're sure there's a bug here, could you give me an example?

Cheers, Daniel

[biovoid]

Original function is below. When I consume all dhara_map_capacity(), but the next write is overwrite of existing page. If we found a page with trace_path then count is not incrementing. But the check is before tracing path, so when full map I'm overwriting page - I receive an error.

static int prepare_write(struct dhara_map *m, dhara_sector_t dst, uint8_t *meta, dhara_error_t *err)
{
    dhara_error_t my_err;

    if (auto_gc(m, err) < 0)
        return 0;

    if (m->count >= dhara_map_capacity(m))
    {
        dhara_set_error(err, DHARA_E_MAP_FULL);
        return -1;
    }

    if (trace_path(m, dst, NULL, meta, &my_err) < 0) 
    {
        if (my_err != DHARA_E_NOT_FOUND) 
        {
            dhara_set_error(err, my_err);
            return -1;
        }        

        m->count++;
    }

    ck_set_count(dhara_journal_cookie(&m->journal), m->count);
    return 0;
}

Proposed check:

static int prepare_write(struct dhara_map *m, dhara_sector_t dst, uint8_t *meta, dhara_error_t *err)
{
    dhara_error_t my_err;

    if (auto_gc(m, err) < 0)
        return 0; 

    if (trace_path(m, dst, NULL, meta, &my_err) < 0) 
    {
        if (my_err != DHARA_E_NOT_FOUND) 
        {
            dhara_set_error(err, my_err);
            return -1;
        }        

        if (m->count >= dhara_map_capacity(m))
        {
            dhara_set_error(err, DHARA_E_MAP_FULL);
            return -1;
        }

        m->count++;
    }

    ck_set_count(dhara_journal_cookie(&m->journal), m->count);
    return 0;
}

[dlbeer]

On Wed, Apr 15, 2020 at 01:38:24AM -0700, Sergey Kashlikov wrote:

Original function is below. When I consume all dhara_map_capacity(), but the next write is overwrite of existing page. If we found a page with trace_path then count is not incrementing. But the check is before tracing path, so when full map I'm overwriting page - I receive an error.

static int prepare_write(struct dhara_map *m, dhara_sector_t dst, uint8_t *meta, dhara_error_t *err)
{
    dhara_error_t my_err;

    if (auto_gc(m, err) < 0)
        return 0;

    if (m->count >= dhara_map_capacity(m))
    {
        dhara_set_error(err, DHARA_E_MAP_FULL);
        return -1;
    }

    if (trace_path(m, dst, NULL, meta, &my_err) < 0) 
    {
        if (my_err != DHARA_E_NOT_FOUND) 
        {
            dhara_set_error(err, my_err);
            return -1;
        }        

        m->count++;
    }

    ck_set_count(dhara_journal_cookie(&m->journal), m->count);
    return 0;
}

Proposed check:

static int prepare_write(struct dhara_map *m, dhara_sector_t dst, uint8_t *meta, dhara_error_t *err)
{
    dhara_error_t my_err;

    if (auto_gc(m, err) < 0)
        return 0; 

    if (trace_path(m, dst, NULL, meta, &my_err) < 0) 
    {
        if (my_err != DHARA_E_NOT_FOUND) 
        {
            dhara_set_error(err, my_err);
            return -1;
        }        

        if (m->count >= dhara_map_capacity(m))
        {
            dhara_set_error(err, DHARA_E_MAP_FULL);
            return -1;
        }

        m->count++;
    }

    ck_set_count(dhara_journal_cookie(&m->journal), m->count);
    return 0;
}

Ah! Yes, I see what you mean. That's a good idea -- I'll make this change now.

-- Daniel Beer dlbeer@gmail.com http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B