Skip to content

Commit

Permalink
Fixed issue where a rename causes a split and pushes dir out of sync
Browse files Browse the repository at this point in the history 
The issue happens when a rename causes a split in the destination pair.
If the destination pair is the same as the source pair, this triggers the
logic to keep both pairs in sync. Unfortunately, this logic didn't work,
because the source entry still resides in the old source pair, unlike
the destination pair, which is now in the new pair created by the split.

The best fix for now is to refetch the source pair after the changes to the
destination pair. This isn't the most efficient solution, but fortunately
this bug has already been fixed in the revamped move logic in littlefs v2
(currently in progress).

Found by ohoc
  • Loading branch information
@geky
geky committed Oct 20, 2018
1 parent 0bb1f7a commit 97d8d5e
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 23 deletions.
39 changes: 18 additions & 21 deletions lfs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -888,7 +888,7 @@ static int lfs_dir_find(lfs_t *lfs, lfs_dir_t *dir,
}

// check that entry has not been moved
if (entry->d.type & 0x80) {
if (!lfs->moving && entry->d.type & 0x80) {
int moved = lfs_moved(lfs, &entry->d.u);
if (moved < 0 || moved) {
return (moved < 0) ? moved : LFS_ERR_NOENT;
Expand Down Expand Up @@ -1922,7 +1922,14 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) {
// find old entry
lfs_dir_t oldcwd;
lfs_entry_t oldentry;
int err = lfs_dir_find(lfs, &oldcwd, &oldentry, &oldpath);
int err = lfs_dir_find(lfs, &oldcwd, &oldentry, &(const char *){oldpath});
if (err) {
return err;
}

// mark as moving
oldentry.d.type |= 0x80;
err = lfs_dir_update(lfs, &oldcwd, &oldentry, NULL);
if (err) {
return err;
}
Expand All @@ -1935,11 +1942,9 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) {
return err;
}

bool prevexists = (err != LFS_ERR_NOENT);
bool samepair = (lfs_paircmp(oldcwd.pair, newcwd.pair) == 0);

// must have same type
if (prevexists && preventry.d.type != oldentry.d.type) {
bool prevexists = (err != LFS_ERR_NOENT);
if (prevexists && preventry.d.type != (0x7f & oldentry.d.type)) {
return LFS_ERR_ISDIR;
}

Expand All @@ -1956,18 +1961,6 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) {
}
}

// mark as moving
oldentry.d.type |= 0x80;
err = lfs_dir_update(lfs, &oldcwd, &oldentry, NULL);
if (err) {
return err;
}

// update pair if newcwd == oldcwd
if (samepair) {
newcwd = oldcwd;
}

// move to new location
lfs_entry_t newentry = preventry;
newentry.d = oldentry.d;
Expand All @@ -1986,10 +1979,13 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) {
}
}

// update pair if newcwd == oldcwd
if (samepair) {
oldcwd = newcwd;
// fetch old pair again in case dir block changed
lfs->moving = true;
err = lfs_dir_find(lfs, &oldcwd, &oldentry, &oldpath);
if (err) {
return err;
}
lfs->moving = false;

// remove old entry
err = lfs_dir_remove(lfs, &oldcwd, &oldentry);
Expand Down Expand Up @@ -2087,6 +2083,7 @@ static int lfs_init(lfs_t *lfs, const struct lfs_config *cfg) {
lfs->files = NULL;
lfs->dirs = NULL;
lfs->deorphaned = false;
lfs->moving = false;

return 0;

Expand Down
1 change: 1 addition & 0 deletions lfs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ typedef struct lfs {

lfs_free_t free;
bool deorphaned;
bool moving;
} lfs_t;


Expand Down
63 changes: 61 additions & 2 deletions tests/test_dirs.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,13 +326,42 @@ tests/test.py << TEST
lfs_unmount(&lfs) => 0;
TEST

echo "--- Multi-block rename ---"
tests/test.py << TEST
lfs_mount(&lfs, &cfg) => 0;
for (int i = 0; i < $LARGESIZE; i++) {
sprintf((char*)buffer, "cactus/test%d", i);
sprintf((char*)wbuffer, "cactus/tedd%d", i);
lfs_rename(&lfs, (char*)buffer, (char*)wbuffer) => 0;
}
lfs_unmount(&lfs) => 0;
TEST
tests/test.py << TEST
lfs_mount(&lfs, &cfg) => 0;
lfs_dir_open(&lfs, &dir[0], "cactus") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, ".") => 0;
info.type => LFS_TYPE_DIR;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "..") => 0;
info.type => LFS_TYPE_DIR;
for (int i = 0; i < $LARGESIZE; i++) {
sprintf((char*)buffer, "tedd%d", i);
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, (char*)buffer) => 0;
info.type => LFS_TYPE_DIR;
}
lfs_dir_read(&lfs, &dir[0], &info) => 0;
lfs_unmount(&lfs) => 0;
TEST

echo "--- Multi-block remove ---"
tests/test.py << TEST
lfs_mount(&lfs, &cfg) => 0;
lfs_remove(&lfs, "cactus") => LFS_ERR_NOTEMPTY;
for (int i = 0; i < $LARGESIZE; i++) {
sprintf((char*)buffer, "cactus/test%d", i);
sprintf((char*)buffer, "cactus/tedd%d", i);
lfs_remove(&lfs, (char*)buffer) => 0;
}
Expand Down Expand Up @@ -391,13 +420,43 @@ tests/test.py << TEST
lfs_unmount(&lfs) => 0;
TEST

echo "--- Multi-block rename with files ---"
tests/test.py << TEST
lfs_mount(&lfs, &cfg) => 0;
for (int i = 0; i < $LARGESIZE; i++) {
sprintf((char*)buffer, "prickly-pear/test%d", i);
sprintf((char*)wbuffer, "prickly-pear/tedd%d", i);
lfs_rename(&lfs, (char*)buffer, (char*)wbuffer) => 0;
}
lfs_unmount(&lfs) => 0;
TEST
tests/test.py << TEST
lfs_mount(&lfs, &cfg) => 0;
lfs_dir_open(&lfs, &dir[0], "prickly-pear") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, ".") => 0;
info.type => LFS_TYPE_DIR;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "..") => 0;
info.type => LFS_TYPE_DIR;
for (int i = 0; i < $LARGESIZE; i++) {
sprintf((char*)buffer, "tedd%d", i);
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, (char*)buffer) => 0;
info.type => LFS_TYPE_REG;
info.size => 6;
}
lfs_dir_read(&lfs, &dir[0], &info) => 0;
lfs_unmount(&lfs) => 0;
TEST

echo "--- Multi-block remove with files ---"
tests/test.py << TEST
lfs_mount(&lfs, &cfg) => 0;
lfs_remove(&lfs, "prickly-pear") => LFS_ERR_NOTEMPTY;
for (int i = 0; i < $LARGESIZE; i++) {
sprintf((char*)buffer, "prickly-pear/test%d", i);
sprintf((char*)buffer, "prickly-pear/tedd%d", i);
lfs_remove(&lfs, (char*)buffer) => 0;
}
Expand Down

0 comments on commit 97d8d5e

Please sign in to comment.