| Message ID | 20181023094147.7906-2-suy.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: fixes of file extent in original and lowmem check | expand |
On 2018/10/23 下午5:41, Su Yue wrote: > Since repair will do CoW, the outer path may be invalid, > add an argument path to punch_extent_hole(). > When punch_extent_hole() returns, path will still point to the item > before calling punch_extent_hole(); > > Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com> Overall it looks OK, however still some nitpicks with a new bug exposed inlined below. > --- > check/mode-lowmem.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c > index 1bce44f5658a..c8e4f13d816f 100644 > --- a/check/mode-lowmem.c > +++ b/check/mode-lowmem.c > @@ -1710,15 +1710,20 @@ out: > /* > * Wrapper function of btrfs_punch_hole. > * > + * @path: will point to the item while calling the function. > + * > * Returns 0 means success. > * Returns not 0 means error. > */ > -static int punch_extent_hole(struct btrfs_root *root, u64 ino, u64 start, > - u64 len) > +static int punch_extent_hole(struct btrfs_root *root, struct btrfs_path *path, > + u64 ino, u64 start, u64 len) > { > struct btrfs_trans_handle *trans; > - int ret = 0; > + struct btrfs_key key; > + int ret; > + int ret2; I didn't see the need to introduce another ret. @ret is used to record the error from btrfs_punch_hole(), which should only return <0 or 0. We should only continue for ret == 0, so we can overwrite @ret. > > + btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]); > trans = btrfs_start_transaction(root, 1); > if (IS_ERR(trans)) > return PTR_ERR(trans); > @@ -1732,6 +1737,12 @@ static int punch_extent_hole(struct btrfs_root *root, u64 ino, u64 start, > ino); > > btrfs_commit_transaction(trans, root); Here since the wrapper is a little old and at that time we don't have btrfs_abort_transaction() (well, not really have one even now). The correct behavior is to abort trans and return error if we get error from btrfs_punch_hole(). And if we get no error from btrfs_punch_hole(), we can reuse @ret for btrfs_search_slot(), no need to introduce @ret2. > + > + btrfs_release_path(path); > + ret2 = btrfs_search_slot(NULL, root, &key, path, 0, 0); > + if (ret2 > 0) > + ret2 = -ENOENT; > + ret |= ret2; > return ret; > } > > @@ -1963,7 +1974,7 @@ static int check_file_extent(struct btrfs_root *root, struct btrfs_path *path, > /* Check EXTENT_DATA hole */ > if (!no_holes && *end != fkey.offset) { > if (repair) > - ret = punch_extent_hole(root, fkey.objectid, > + ret = punch_extent_hole(root, path, fkey.objectid, > *end, fkey.offset - *end); > if (!repair || ret) { > err |= FILE_EXTENT_ERROR; > @@ -2534,7 +2545,7 @@ out: > > if (!nbytes && !no_holes && extent_end < isize) { > if (repair) > - ret = punch_extent_hole(root, inode_id, > + ret = punch_extent_hole(root, path, inode_id, > extent_end, isize - extent_end); In this case, it's check_inode_item() and it's repair case for missing tailing hole. However isize can be unaligned, but hole extent should always be aligned. So there is another bug that repair could create invalid hole extents. Thanks, Qu > if (!repair || ret) { > err |= NBYTES_ERROR; >
On 10/23/18 6:04 PM, Qu Wenruo wrote: > > > On 2018/10/23 下午5:41, Su Yue wrote: >> Since repair will do CoW, the outer path may be invalid, >> add an argument path to punch_extent_hole(). >> When punch_extent_hole() returns, path will still point to the item >> before calling punch_extent_hole(); >> >> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com> > > Overall it looks OK, however still some nitpicks with a new bug exposed > inlined below. > >> --- >> check/mode-lowmem.c | 21 ++++++++++++++++----- >> 1 file changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c >> index 1bce44f5658a..c8e4f13d816f 100644 >> --- a/check/mode-lowmem.c >> +++ b/check/mode-lowmem.c >> @@ -1710,15 +1710,20 @@ out: >> /* >> * Wrapper function of btrfs_punch_hole. >> * >> + * @path: will point to the item while calling the function. >> + * >> * Returns 0 means success. >> * Returns not 0 means error. >> */ >> -static int punch_extent_hole(struct btrfs_root *root, u64 ino, u64 start, >> - u64 len) >> +static int punch_extent_hole(struct btrfs_root *root, struct btrfs_path *path, >> + u64 ino, u64 start, u64 len) >> { >> struct btrfs_trans_handle *trans; >> - int ret = 0; >> + struct btrfs_key key; >> + int ret; >> + int ret2; > > I didn't see the need to introduce another ret. > > @ret is used to record the error from btrfs_punch_hole(), which should > only return <0 or 0. > > We should only continue for ret == 0, so we can overwrite @ret. > >> >> + btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]); >> trans = btrfs_start_transaction(root, 1); >> if (IS_ERR(trans)) >> return PTR_ERR(trans); >> @@ -1732,6 +1737,12 @@ static int punch_extent_hole(struct btrfs_root *root, u64 ino, u64 start, >> ino); >> >> btrfs_commit_transaction(trans, root); > > Here since the wrapper is a little old and at that time we don't have > btrfs_abort_transaction() (well, not really have one even now). > > The correct behavior is to abort trans and return error if we get error > from btrfs_punch_hole(). > > And if we get no error from btrfs_punch_hole(), we can reuse @ret for > btrfs_search_slot(), no need to introduce @ret2. > Thanks to your clean explanation. >> + >> + btrfs_release_path(path); >> + ret2 = btrfs_search_slot(NULL, root, &key, path, 0, 0); >> + if (ret2 > 0) >> + ret2 = -ENOENT; >> + ret |= ret2; >> return ret; >> } >> >> @@ -1963,7 +1974,7 @@ static int check_file_extent(struct btrfs_root *root, struct btrfs_path *path, >> /* Check EXTENT_DATA hole */ >> if (!no_holes && *end != fkey.offset) { >> if (repair) >> - ret = punch_extent_hole(root, fkey.objectid, >> + ret = punch_extent_hole(root, path, fkey.objectid, >> *end, fkey.offset - *end); >> if (!repair || ret) { >> err |= FILE_EXTENT_ERROR; >> @@ -2534,7 +2545,7 @@ out: >> >> if (!nbytes && !no_holes && extent_end < isize) { >> if (repair) >> - ret = punch_extent_hole(root, inode_id, >> + ret = punch_extent_hole(root, path, inode_id, >> extent_end, isize - extent_end); > > In this case, it's check_inode_item() and it's repair case for missing > tailing hole. > > However isize can be unaligned, but hole extent should always be aligned. > So there is another bug that repair could create invalid hole extents. > Oh.. Will fix those two. Thanks, Su > Thanks, > Qu > >> if (!repair || ret) { >> err |= NBYTES_ERROR; >> > >
diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c index 1bce44f5658a..c8e4f13d816f 100644 --- a/check/mode-lowmem.c +++ b/check/mode-lowmem.c @@ -1710,15 +1710,20 @@ out: /* * Wrapper function of btrfs_punch_hole. * + * @path: will point to the item while calling the function. + * * Returns 0 means success. * Returns not 0 means error. */ -static int punch_extent_hole(struct btrfs_root *root, u64 ino, u64 start, - u64 len) +static int punch_extent_hole(struct btrfs_root *root, struct btrfs_path *path, + u64 ino, u64 start, u64 len) { struct btrfs_trans_handle *trans; - int ret = 0; + struct btrfs_key key; + int ret; + int ret2; + btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]); trans = btrfs_start_transaction(root, 1); if (IS_ERR(trans)) return PTR_ERR(trans); @@ -1732,6 +1737,12 @@ static int punch_extent_hole(struct btrfs_root *root, u64 ino, u64 start, ino); btrfs_commit_transaction(trans, root); + + btrfs_release_path(path); + ret2 = btrfs_search_slot(NULL, root, &key, path, 0, 0); + if (ret2 > 0) + ret2 = -ENOENT; + ret |= ret2; return ret; } @@ -1963,7 +1974,7 @@ static int check_file_extent(struct btrfs_root *root, struct btrfs_path *path, /* Check EXTENT_DATA hole */ if (!no_holes && *end != fkey.offset) { if (repair) - ret = punch_extent_hole(root, fkey.objectid, + ret = punch_extent_hole(root, path, fkey.objectid, *end, fkey.offset - *end); if (!repair || ret) { err |= FILE_EXTENT_ERROR; @@ -2534,7 +2545,7 @@ out: if (!nbytes && !no_holes && extent_end < isize) { if (repair) - ret = punch_extent_hole(root, inode_id, + ret = punch_extent_hole(root, path, inode_id, extent_end, isize - extent_end); if (!repair || ret) { err |= NBYTES_ERROR;
Since repair will do CoW, the outer path may be invalid, add an argument path to punch_extent_hole(). When punch_extent_hole() returns, path will still point to the item before calling punch_extent_hole(); Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com> --- check/mode-lowmem.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)