| Message ID | 1415897993-7556-1-git-send-email-fdmanana@suse.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, 13 Nov 2014 16:59:53 +0000, Filipe Manana wrote: > If an error happens during writeback of log btree extents, make sure the > error is returned to the caller (fsync), so that it takes proper action > (commit current transaction) instead of writing a superblock that points > to log btrees with all or some nodes that weren't durably persisted. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > fs/btrfs/tree-log.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c > index 6d58d72..c8274d3 100644 > --- a/fs/btrfs/tree-log.c > +++ b/fs/btrfs/tree-log.c > @@ -2599,12 +2599,14 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans, > index2 = root_log_ctx.log_transid % 2; > if (atomic_read(&log_root_tree->log_commit[index2])) { > blk_finish_plug(&plug); > - btrfs_wait_marked_extents(log, &log->dirty_log_pages, mark); > + ret = btrfs_wait_marked_extents(log, &log->dirty_log_pages, > + mark); > wait_log_commit(trans, log_root_tree, > root_log_ctx.log_transid); > btrfs_free_logged_extents(log, log_transid); > mutex_unlock(&log_root_tree->log_mutex); > - ret = root_log_ctx.log_ret; > + if (!ret) > + ret = root_log_ctx.log_ret; > goto out; > } > ASSERT(root_log_ctx.log_transid == log_root_tree->log_transid); This first hunk didn't apply to my 3.14.x tree that is 99.999% in sync with btrfs-3.18+, as a line is missing from the context. See: https://git.kernel.org/cgit/linux/kernel/git/mason/linux-btrfs.git/tree/fs/btrfs/tree-log.c?h=for-linus#n2605 Any idea where that missing btrfs_free_logged_extents() went? cheers Holger -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 13 Nov 2014 17:33:19 +0000, Holger Hoffstätte wrote: > On Thu, 13 Nov 2014 16:59:53 +0000, Filipe Manana wrote: > >> If an error happens during writeback of log btree extents, make sure the >> error is returned to the caller (fsync), so that it takes proper action >> (commit current transaction) instead of writing a superblock that points >> to log btrees with all or some nodes that weren't durably persisted. >> >> Signed-off-by: Filipe Manana <fdmanana@suse.com> >> --- >> fs/btrfs/tree-log.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c >> index 6d58d72..c8274d3 100644 >> --- a/fs/btrfs/tree-log.c >> +++ b/fs/btrfs/tree-log.c >> @@ -2599,12 +2599,14 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans, >> index2 = root_log_ctx.log_transid % 2; >> if (atomic_read(&log_root_tree->log_commit[index2])) { >> blk_finish_plug(&plug); >> - btrfs_wait_marked_extents(log, &log->dirty_log_pages, mark); >> + ret = btrfs_wait_marked_extents(log, &log->dirty_log_pages, >> + mark); >> wait_log_commit(trans, log_root_tree, >> root_log_ctx.log_transid); >> btrfs_free_logged_extents(log, log_transid); >> mutex_unlock(&log_root_tree->log_mutex); >> - ret = root_log_ctx.log_ret; >> + if (!ret) >> + ret = root_log_ctx.log_ret; >> goto out; >> } >> ASSERT(root_log_ctx.log_transid == log_root_tree->log_transid); > > This first hunk didn't apply to my 3.14.x tree that is 99.999% in sync with > btrfs-3.18+, as a line is missing from the context. See: > > https://git.kernel.org/cgit/linux/kernel/git/mason/linux-btrfs.git/tree/fs/btrfs/tree-log.c?h=for-linus#n2605 > > Any idea where that missing btrfs_free_logged_extents() went? Found it: it went away with Josef's recent patch on Nov 6: "Btrfs: make sure we wait on logged extents when fsycning two subvols" (http://permalink.gmane.org/gmane.comp.file-systems.btrfs/40005) ..which I have applied. boohoo. That patch also adds another call to btrfs_wait_marked_extents(), which should then probably also have its return value handled? thanks, Holger -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 13, 2014 at 5:33 PM, Holger Hoffstätte <holger.hoffstaette@googlemail.com> wrote: > On Thu, 13 Nov 2014 16:59:53 +0000, Filipe Manana wrote: > >> If an error happens during writeback of log btree extents, make sure the >> error is returned to the caller (fsync), so that it takes proper action >> (commit current transaction) instead of writing a superblock that points >> to log btrees with all or some nodes that weren't durably persisted. >> >> Signed-off-by: Filipe Manana <fdmanana@suse.com> >> --- >> fs/btrfs/tree-log.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c >> index 6d58d72..c8274d3 100644 >> --- a/fs/btrfs/tree-log.c >> +++ b/fs/btrfs/tree-log.c >> @@ -2599,12 +2599,14 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans, >> index2 = root_log_ctx.log_transid % 2; >> if (atomic_read(&log_root_tree->log_commit[index2])) { >> blk_finish_plug(&plug); >> - btrfs_wait_marked_extents(log, &log->dirty_log_pages, mark); >> + ret = btrfs_wait_marked_extents(log, &log->dirty_log_pages, >> + mark); >> wait_log_commit(trans, log_root_tree, >> root_log_ctx.log_transid); >> btrfs_free_logged_extents(log, log_transid); >> mutex_unlock(&log_root_tree->log_mutex); >> - ret = root_log_ctx.log_ret; >> + if (!ret) >> + ret = root_log_ctx.log_ret; >> goto out; >> } >> ASSERT(root_log_ctx.log_transid == log_root_tree->log_transid); > > This first hunk didn't apply to my 3.14.x tree that is 99.999% in sync with > btrfs-3.18+, as a line is missing from the context. See: > > https://git.kernel.org/cgit/linux/kernel/git/mason/linux-btrfs.git/tree/fs/btrfs/tree-log.c?h=for-linus#n2605 > > Any idea where that missing btrfs_free_logged_extents() went? Probably because you picked the following patch: https://patchwork.kernel.org/patch/5244311/ While Chris' integration/for-linus branches don't have the patch included yet (nor do I in my local branch). Nevertheless, it's a trivial conflict to solve, just leave Josef's changes and mine. thanks > > cheers > Holger > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 13, 2014 at 5:43 PM, Holger Hoffstätte <holger.hoffstaette@googlemail.com> wrote: > On Thu, 13 Nov 2014 17:33:19 +0000, Holger Hoffstätte wrote: > >> On Thu, 13 Nov 2014 16:59:53 +0000, Filipe Manana wrote: >> >>> If an error happens during writeback of log btree extents, make sure the >>> error is returned to the caller (fsync), so that it takes proper action >>> (commit current transaction) instead of writing a superblock that points >>> to log btrees with all or some nodes that weren't durably persisted. >>> >>> Signed-off-by: Filipe Manana <fdmanana@suse.com> >>> --- >>> fs/btrfs/tree-log.c | 21 +++++++++++++++------ >>> 1 file changed, 15 insertions(+), 6 deletions(-) >>> >>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c >>> index 6d58d72..c8274d3 100644 >>> --- a/fs/btrfs/tree-log.c >>> +++ b/fs/btrfs/tree-log.c >>> @@ -2599,12 +2599,14 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans, >>> index2 = root_log_ctx.log_transid % 2; >>> if (atomic_read(&log_root_tree->log_commit[index2])) { >>> blk_finish_plug(&plug); >>> - btrfs_wait_marked_extents(log, &log->dirty_log_pages, mark); >>> + ret = btrfs_wait_marked_extents(log, &log->dirty_log_pages, >>> + mark); >>> wait_log_commit(trans, log_root_tree, >>> root_log_ctx.log_transid); >>> btrfs_free_logged_extents(log, log_transid); >>> mutex_unlock(&log_root_tree->log_mutex); >>> - ret = root_log_ctx.log_ret; >>> + if (!ret) >>> + ret = root_log_ctx.log_ret; >>> goto out; >>> } >>> ASSERT(root_log_ctx.log_transid == log_root_tree->log_transid); >> >> This first hunk didn't apply to my 3.14.x tree that is 99.999% in sync with >> btrfs-3.18+, as a line is missing from the context. See: >> >> https://git.kernel.org/cgit/linux/kernel/git/mason/linux-btrfs.git/tree/fs/btrfs/tree-log.c?h=for-linus#n2605 >> >> Any idea where that missing btrfs_free_logged_extents() went? > > Found it: it went away with Josef's recent patch on Nov 6: > "Btrfs: make sure we wait on logged extents when fsycning two subvols" > (http://permalink.gmane.org/gmane.comp.file-systems.btrfs/40005) > > ..which I have applied. boohoo. > > That patch also adds another call to btrfs_wait_marked_extents(), which should > then probably also have its return value handled? No, it's a different call - btrfs_wait_logged_extents(). > > thanks, > Holger > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 13 Nov 2014 17:50:44 +0000, Filipe David Manana wrote: [snip] >>> This first hunk didn't apply to my 3.14.x tree that is 99.999% in sync with >>> btrfs-3.18+, as a line is missing from the context. See: >>> >>> https://git.kernel.org/cgit/linux/kernel/git/mason/linux-btrfs.git/tree/fs/btrfs/tree-log.c?h=for-linus#n2605 >>> >>> Any idea where that missing btrfs_free_logged_extents() went? >> >> Found it: it went away with Josef's recent patch on Nov 6: >> "Btrfs: make sure we wait on logged extents when fsycning two subvols" >> (http://permalink.gmane.org/gmane.comp.file-systems.btrfs/40005) >> >> ..which I have applied. boohoo. >> >> That patch also adds another call to btrfs_wait_marked_extents(), which should >> then probably also have its return value handled? > > No, it's a different call - btrfs_wait_logged_extents(). Of course, you are right. I can't read (it's late :) cheers -h -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 13 Nov 2014 17:47:51 +0000, Filipe David Manana wrote: [snip] > While Chris' integration/for-linus branches don't have the patch > included yet (nor do I in my local branch). Nevertheless, it's a > trivial conflict to solve, just leave Josef's changes and mine. So if I read this correctly the combined patch should look like this, right? --snip-- - btrfs_wait_marked_extents(log, &log->dirty_log_pages, mark); + ret = btrfs_wait_marked_extents(log, &log->dirty_log_pages, + mark); btrfs_wait_logged_extents(log, log_transid); wait_log_commit(trans, log_root_tree, root_log_ctx.log_transid); mutex_unlock(&log_root_tree->log_mutex); - ret = root_log_ctx.log_ret; + if (!ret) + ret = root_log_ctx.log_ret; goto out; --snip-- This saves the ret, logs the extents, commits and then handles any error. -h -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index 6d58d72..c8274d3 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c @@ -2599,12 +2599,14 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans, index2 = root_log_ctx.log_transid % 2; if (atomic_read(&log_root_tree->log_commit[index2])) { blk_finish_plug(&plug); - btrfs_wait_marked_extents(log, &log->dirty_log_pages, mark); + ret = btrfs_wait_marked_extents(log, &log->dirty_log_pages, + mark); wait_log_commit(trans, log_root_tree, root_log_ctx.log_transid); btrfs_free_logged_extents(log, log_transid); mutex_unlock(&log_root_tree->log_mutex); - ret = root_log_ctx.log_ret; + if (!ret) + ret = root_log_ctx.log_ret; goto out; } ASSERT(root_log_ctx.log_transid == log_root_tree->log_transid); @@ -2641,10 +2643,17 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans, mutex_unlock(&log_root_tree->log_mutex); goto out_wake_log_root; } - btrfs_wait_marked_extents(log, &log->dirty_log_pages, mark); - btrfs_wait_marked_extents(log_root_tree, - &log_root_tree->dirty_log_pages, - EXTENT_NEW | EXTENT_DIRTY); + ret = btrfs_wait_marked_extents(log, &log->dirty_log_pages, mark); + if (!ret) + ret = btrfs_wait_marked_extents(log_root_tree, + &log_root_tree->dirty_log_pages, + EXTENT_NEW | EXTENT_DIRTY); + if (ret) { + btrfs_set_log_full_commit(root->fs_info, trans); + btrfs_free_logged_extents(log, log_transid); + mutex_unlock(&log_root_tree->log_mutex); + goto out_wake_log_root; + } btrfs_wait_logged_extents(log, log_transid); btrfs_set_super_log_root(root->fs_info->super_for_commit,
If an error happens during writeback of log btree extents, make sure the error is returned to the caller (fsync), so that it takes proper action (commit current transaction) instead of writing a superblock that points to log btrees with all or some nodes that weren't durably persisted. Signed-off-by: Filipe Manana <fdmanana@suse.com> --- fs/btrfs/tree-log.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)