| Message ID | 1247199830-21179-1-git-send-email-tytso@mit.edu |
|---|---|
| State | Superseded, archived |
| Headers | show |
Theodore Ts'o wrote: > In the case where we ext2fs_extent_set_bmap() is replacing the block > mapping at the beginning of an already-existing extent, insert a new > extent if necessary before shrinking an existing extent, to avoid data > loss if the disk is full. > > This mostly addresses the problem described in Red Hat Bugzilla's > statistics are still wrong, but at least the files on the filesystem > are not corrupted. If there is a failure during the > inode_scan_and_fix pass, the simplest thing to do may be to tell the > user to run e2fsck -fy. > > Addresses-Red-Hat-Bug: #510379 Thanks Ted, looks reasonable at first glance. I'll review more in depth tomorrow, but I think you can also remove the again: target, since nobody's going there anymore w/ this patch. -Eric > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > --- > lib/ext2fs/extent.c | 44 ++++++++++++++++++++++++++++++++------------ > 1 files changed, 32 insertions(+), 12 deletions(-) > > diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c > index 4a4fd2c..2f280ea 100644 > --- a/lib/ext2fs/extent.c > +++ b/lib/ext2fs/extent.c > @@ -1270,24 +1270,44 @@ again: > #ifdef DEBUG > printf("(re/un)mapping first block in extent\n"); > #endif > + if (physical) { > + retval = ext2fs_extent_get(handle, > + EXT2_EXTENT_PREV_LEAF, > + &extent); > + if (extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT) > + extent_uninit = 1; > + if (retval == EXT2_ET_EXTENT_NO_PREV) { > + retval = ext2fs_extent_insert(handle, > + 0, &newextent); > + } else if (retval) > + goto done; > + else if ((logical == extent.e_lblk + extent.e_len) && > + (physical == extent.e_pblk + extent.e_len) && > + (new_uninit == extent_uninit) && > + ((int) extent.e_len < max_len-1)) { > + extent.e_len++; > + retval = ext2fs_extent_replace(handle, 0, > + &extent); > + } else { > + retval = ext2fs_extent_insert(handle, > + EXT2_EXTENT_INSERT_AFTER, &newextent); > + } > + if (retval) > + goto done; > + } > + retval = ext2fs_extent_fix_parents(handle); > + if (retval) > + goto done; > + retval = ext2fs_extent_get(handle, EXT2_EXTENT_NEXT_LEAF, > + &extent); > + if (retval) > + goto done; > extent.e_pblk++; > extent.e_lblk++; > extent.e_len--; > retval = ext2fs_extent_replace(handle, 0, &extent); > if (retval) > goto done; > - if (physical) { > - /* > - * We've removed the old block, now rely on > - * the optimized hueristics for adding a new > - * mapping with appropriate merging if necessary. > - */ > - goto again; > - } else { > - retval = ext2fs_extent_fix_parents(handle); > - if (retval) > - goto done; > - } > } else { > __u32 orig_length; > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Theodore Ts'o wrote: > In the case where we ext2fs_extent_set_bmap() is replacing the block > mapping at the beginning of an already-existing extent, insert a new > extent if necessary before shrinking an existing extent, to avoid data > loss if the disk is full. > > This mostly addresses the problem described in Red Hat Bugzilla's > statistics are still wrong, but at least the files on the filesystem > are not corrupted. If there is a failure during the > inode_scan_and_fix pass, the simplest thing to do may be to tell the > user to run e2fsck -fy. > > Addresses-Red-Hat-Bug: #510379 > > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > --- > lib/ext2fs/extent.c | 44 ++++++++++++++++++++++++++++++++------------ > 1 files changed, 32 insertions(+), 12 deletions(-) > > diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c > index 4a4fd2c..2f280ea 100644 > --- a/lib/ext2fs/extent.c > +++ b/lib/ext2fs/extent.c > @@ -1270,24 +1270,44 @@ again: > #ifdef DEBUG > printf("(re/un)mapping first block in extent\n"); > #endif > + if (physical) { > + retval = ext2fs_extent_get(handle, > + EXT2_EXTENT_PREV_LEAF, > + &extent); > + if (extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT) > + extent_uninit = 1; > + if (retval == EXT2_ET_EXTENT_NO_PREV) { > + retval = ext2fs_extent_insert(handle, > + 0, &newextent); > + } else if (retval) > + goto done; A lot of this is cut & paste from "mapping unmapped logical block" - at some point we should probably factor out into helpers, but fine for now. A little comment here about extent merging may be helpful just to guide the reader; it's a lot of conditionals to eyeball - not so difficult in the end, but not all that conducive to a quick skim either. Aside from those nitpicks (and the now-extraneous again: goto target) this look fine to me, thanks. -Eric > + else if ((logical == extent.e_lblk + extent.e_len) && > + (physical == extent.e_pblk + extent.e_len) && > + (new_uninit == extent_uninit) && > + ((int) extent.e_len < max_len-1)) { > + extent.e_len++; > + retval = ext2fs_extent_replace(handle, 0, > + &extent); > + } else { > + retval = ext2fs_extent_insert(handle, > + EXT2_EXTENT_INSERT_AFTER, &newextent); > + } > + if (retval) > + goto done; > + } > + retval = ext2fs_extent_fix_parents(handle); > + if (retval) > + goto done; > + retval = ext2fs_extent_get(handle, EXT2_EXTENT_NEXT_LEAF, > + &extent); > + if (retval) > + goto done; > extent.e_pblk++; > extent.e_lblk++; > extent.e_len--; > retval = ext2fs_extent_replace(handle, 0, &extent); > if (retval) > goto done; > - if (physical) { > - /* > - * We've removed the old block, now rely on > - * the optimized hueristics for adding a new > - * mapping with appropriate merging if necessary. > - */ > - goto again; > - } else { > - retval = ext2fs_extent_fix_parents(handle); > - if (retval) > - goto done; > - } > } else { > __u32 orig_length; > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c index 4a4fd2c..2f280ea 100644 --- a/lib/ext2fs/extent.c +++ b/lib/ext2fs/extent.c @@ -1270,24 +1270,44 @@ again: #ifdef DEBUG printf("(re/un)mapping first block in extent\n"); #endif + if (physical) { + retval = ext2fs_extent_get(handle, + EXT2_EXTENT_PREV_LEAF, + &extent); + if (extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT) + extent_uninit = 1; + if (retval == EXT2_ET_EXTENT_NO_PREV) { + retval = ext2fs_extent_insert(handle, + 0, &newextent); + } else if (retval) + goto done; + else if ((logical == extent.e_lblk + extent.e_len) && + (physical == extent.e_pblk + extent.e_len) && + (new_uninit == extent_uninit) && + ((int) extent.e_len < max_len-1)) { + extent.e_len++; + retval = ext2fs_extent_replace(handle, 0, + &extent); + } else { + retval = ext2fs_extent_insert(handle, + EXT2_EXTENT_INSERT_AFTER, &newextent); + } + if (retval) + goto done; + } + retval = ext2fs_extent_fix_parents(handle); + if (retval) + goto done; + retval = ext2fs_extent_get(handle, EXT2_EXTENT_NEXT_LEAF, + &extent); + if (retval) + goto done; extent.e_pblk++; extent.e_lblk++; extent.e_len--; retval = ext2fs_extent_replace(handle, 0, &extent); if (retval) goto done; - if (physical) { - /* - * We've removed the old block, now rely on - * the optimized hueristics for adding a new - * mapping with appropriate merging if necessary. - */ - goto again; - } else { - retval = ext2fs_extent_fix_parents(handle); - if (retval) - goto done; - } } else { __u32 orig_length;
In the case where we ext2fs_extent_set_bmap() is replacing the block mapping at the beginning of an already-existing extent, insert a new extent if necessary before shrinking an existing extent, to avoid data loss if the disk is full. This mostly addresses the problem described in Red Hat Bugzilla's statistics are still wrong, but at least the files on the filesystem are not corrupted. If there is a failure during the inode_scan_and_fix pass, the simplest thing to do may be to tell the user to run e2fsck -fy. Addresses-Red-Hat-Bug: #510379 Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- lib/ext2fs/extent.c | 44 ++++++++++++++++++++++++++++++++------------ 1 files changed, 32 insertions(+), 12 deletions(-)