dlbeer / dhara

NAND flash translation layer for low-memory systems
Other
409 stars 120 forks source link

prepare_write behavior #12

Closed biovoid closed 4 years ago

biovoid commented 4 years ago

Hi,

Why inside the prepare_write() function the auto_gc() check returns 0 in case of error? Look at code section:

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; <<< Why this is zero and not -1? 

    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;
}
dlbeer commented 4 years ago

On Sun, Apr 19, 2020 at 05:13:42AM -0700, Sergey Kashlikov wrote:

Hi,

Why inside the prepare_write() function the auto_gc() check returns 0 in case of error? Look at code section:


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; <<< Why this is zero and not -1? 

Hi Sergey,

If auto_gc() fails, the error is not immediately fatal (but perhaps it should be?). It means that the map will run out of space earlier in future, but it doesn't necessarily mean that the current write will fail.

Cheers, Daniel

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

biovoid commented 4 years ago

If you want "soft" error handling in that case then you need to remove that check for auto_gc. Code returns with 0, but trace_path and ck_set_count would not be called. I prefer to return error in case of failure, earlier occurs - earlier fixed. If nand interface or memory is broken that leads to auto_gc failure then saving writes make no sense (for me).

dlbeer commented 4 years ago

On Mon, Apr 20, 2020 at 06:41:01AM -0700, Sergey Kashlikov wrote:

If you want "soft" error handling in that case then you need to remove that check for auto_gc. Code returns with 0, but trace_path and ck_set_count would not be called. I prefer to return error in case of failure, earlier occurs - earlier fixed. If nand interface or memory is broken that leads to auto_gc failure then saving writes make no sense (for me).

Yes, you're right -- I wonder now if maybe that was actually a typo (it was a few years ago), because that behaviour doesn't make any sense.

Fixed now.

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