| Message ID | 1390021080-30788-1-git-send-email-wangsl.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Sat, Jan 18, 2014 at 12:58:00PM +0800, Wang Shilong wrote: > Steps to reproduce: > # mkfs.btrfs -f /dev/sda8 > # mount /dev/sda8 /mnt > # btrfs sub snapshot -r /mnt /mnt/snap1 > # btrfs sub snapshot -r /mnt /mnt/snap2 > # btrfs send /mnt/snap1 -p /mnt/snap2 -f /mnt/1 > # dmesg > > The problem is that we will sort clone roots(include @send_root), it > might push @send_root before thus @send_root's @send_in_progress will > be decreased twice. > > Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> Reviewed-by: David Sterba <dsterba@suse.cz> -- 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 Sat, Jan 18, 2014 at 4:58 AM, Wang Shilong <wangsl.fnst@cn.fujitsu.com> wrote: > Steps to reproduce: > # mkfs.btrfs -f /dev/sda8 > # mount /dev/sda8 /mnt > # btrfs sub snapshot -r /mnt /mnt/snap1 > # btrfs sub snapshot -r /mnt /mnt/snap2 > # btrfs send /mnt/snap1 -p /mnt/snap2 -f /mnt/1 > # dmesg > > The problem is that we will sort clone roots(include @send_root), it > might push @send_root before thus @send_root's @send_in_progress will > be decreased twice. > > Cc: David Sterba <dsterba@suse.cz> > Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> > --- > changelog > v2->v3: > add comments as david addressed. > v1->v2: > use right root to check > --- > fs/btrfs/send.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > index bff0b1a..83b6bdb 100644 > --- a/fs/btrfs/send.c > +++ b/fs/btrfs/send.c > @@ -4752,6 +4752,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) > u32 i; > u64 *clone_sources_tmp = NULL; > int clone_sources_to_rollback = 0; > + int sort_clone_roots = 0; > > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > @@ -4942,6 +4943,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) > sort(sctx->clone_roots, sctx->clone_roots_cnt, > sizeof(*sctx->clone_roots), __clone_root_cmp_sort, > NULL); > + sort_clone_roots = 1; > > ret = send_subvol(sctx); > if (ret < 0) > @@ -4957,11 +4959,22 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) > } > > out: > - for (i = 0; sctx && i < clone_sources_to_rollback; i++) > - btrfs_root_dec_send_in_progress(sctx->clone_roots[i].root); > + if (sort_clone_roots) { > + for (i = 0; i < sctx->clone_roots_cnt; i++) > + btrfs_root_dec_send_in_progress( > + sctx->clone_roots[i].root); sctx can be NULL here, anywhere after the 'out' label. > + } else { > + for (i = 0; sctx && i < clone_sources_to_rollback; i++) > + btrfs_root_dec_send_in_progress( > + sctx->clone_roots[i].root); Same here. > + /* > + * if we fail to add @send_root in clone roots, we still > + * need to decrease @send_in_progress count here. > + */ > + btrfs_root_dec_send_in_progress(send_root); > + } > if (sctx && !IS_ERR_OR_NULL(sctx->parent_root)) > btrfs_root_dec_send_in_progress(sctx->parent_root); > - btrfs_root_dec_send_in_progress(send_root); > > kfree(arg); > vfree(clone_sources_tmp); > -- > 1.8.3.1 > > -- > 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
Hello Filipe, > On Sat, Jan 18, 2014 at 4:58 AM, Wang Shilong > <wangsl.fnst@cn.fujitsu.com> wrote: >> Steps to reproduce: >> # mkfs.btrfs -f /dev/sda8 >> # mount /dev/sda8 /mnt >> # btrfs sub snapshot -r /mnt /mnt/snap1 >> # btrfs sub snapshot -r /mnt /mnt/snap2 >> # btrfs send /mnt/snap1 -p /mnt/snap2 -f /mnt/1 >> # dmesg >> >> The problem is that we will sort clone roots(include @send_root), it >> might push @send_root before thus @send_root's @send_in_progress will >> be decreased twice. >> >> Cc: David Sterba <dsterba@suse.cz> >> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> >> --- >> changelog >> v2->v3: >> add comments as david addressed. >> v1->v2: >> use right root to check >> --- >> fs/btrfs/send.c | 19 ++++++++++++++++--- >> 1 file changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c >> index bff0b1a..83b6bdb 100644 >> --- a/fs/btrfs/send.c >> +++ b/fs/btrfs/send.c >> @@ -4752,6 +4752,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) >> u32 i; >> u64 *clone_sources_tmp = NULL; >> int clone_sources_to_rollback = 0; >> + int sort_clone_roots = 0; >> >> if (!capable(CAP_SYS_ADMIN)) >> return -EPERM; >> @@ -4942,6 +4943,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) >> sort(sctx->clone_roots, sctx->clone_roots_cnt, >> sizeof(*sctx->clone_roots), __clone_root_cmp_sort, >> NULL); >> + sort_clone_roots = 1; >> >> ret = send_subvol(sctx); >> if (ret < 0) >> @@ -4957,11 +4959,22 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) >> } >> >> out: >> - for (i = 0; sctx && i < clone_sources_to_rollback; i++) >> - btrfs_root_dec_send_in_progress(sctx->clone_roots[i].root); >> + if (sort_clone_roots) { >> + for (i = 0; i < sctx->clone_roots_cnt; i++) >> + btrfs_root_dec_send_in_progress( >> + sctx->clone_roots[i].root); > > sctx can be NULL here, anywhere after the 'out' label. Not really, if @sort_clone_roots is set which means we must have allocated sctx successfully. > >> + } else { >> + for (i = 0; sctx && i < clone_sources_to_rollback; i++) >> + btrfs_root_dec_send_in_progress( >> + sctx->clone_roots[i].root); > > Same here. Notice there is a check above, sctx && i < clone_sources_to_rollback I really took care of that when i wrote the patch, please correct me if i miss something here^_^. Thanks, Wang > >> + /* >> + * if we fail to add @send_root in clone roots, we still >> + * need to decrease @send_in_progress count here. >> + */ >> + btrfs_root_dec_send_in_progress(send_root); >> + } >> if (sctx && !IS_ERR_OR_NULL(sctx->parent_root)) >> btrfs_root_dec_send_in_progress(sctx->parent_root); >> - btrfs_root_dec_send_in_progress(send_root); >> >> kfree(arg); >> vfree(clone_sources_tmp); >> -- >> 1.8.3.1 >> >> -- >> 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 > > > > -- > Filipe David Manana, > > "Reasonable men adapt themselves to the world. > Unreasonable men adapt the world to themselves. > That's why all progress depends on unreasonable men." > -- > 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 -- 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 Tue, Jan 21, 2014 at 4:45 PM, Wang Shilong <wangshilong1991@gmail.com> wrote: > > Hello Filipe, > >> On Sat, Jan 18, 2014 at 4:58 AM, Wang Shilong >> <wangsl.fnst@cn.fujitsu.com> wrote: >>> Steps to reproduce: >>> # mkfs.btrfs -f /dev/sda8 >>> # mount /dev/sda8 /mnt >>> # btrfs sub snapshot -r /mnt /mnt/snap1 >>> # btrfs sub snapshot -r /mnt /mnt/snap2 >>> # btrfs send /mnt/snap1 -p /mnt/snap2 -f /mnt/1 >>> # dmesg >>> >>> The problem is that we will sort clone roots(include @send_root), it >>> might push @send_root before thus @send_root's @send_in_progress will >>> be decreased twice. >>> >>> Cc: David Sterba <dsterba@suse.cz> >>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> >>> --- >>> changelog >>> v2->v3: >>> add comments as david addressed. >>> v1->v2: >>> use right root to check >>> --- >>> fs/btrfs/send.c | 19 ++++++++++++++++--- >>> 1 file changed, 16 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c >>> index bff0b1a..83b6bdb 100644 >>> --- a/fs/btrfs/send.c >>> +++ b/fs/btrfs/send.c >>> @@ -4752,6 +4752,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) >>> u32 i; >>> u64 *clone_sources_tmp = NULL; >>> int clone_sources_to_rollback = 0; >>> + int sort_clone_roots = 0; >>> >>> if (!capable(CAP_SYS_ADMIN)) >>> return -EPERM; >>> @@ -4942,6 +4943,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) >>> sort(sctx->clone_roots, sctx->clone_roots_cnt, >>> sizeof(*sctx->clone_roots), __clone_root_cmp_sort, >>> NULL); >>> + sort_clone_roots = 1; >>> >>> ret = send_subvol(sctx); >>> if (ret < 0) >>> @@ -4957,11 +4959,22 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) >>> } >>> >>> out: >>> - for (i = 0; sctx && i < clone_sources_to_rollback; i++) >>> - btrfs_root_dec_send_in_progress(sctx->clone_roots[i].root); >>> + if (sort_clone_roots) { >>> + for (i = 0; i < sctx->clone_roots_cnt; i++) >>> + btrfs_root_dec_send_in_progress( >>> + sctx->clone_roots[i].root); >> >> sctx can be NULL here, anywhere after the 'out' label. > > Not really, if @sort_clone_roots is set which means we must have allocated > sctx successfully. Right missed that, same below. Static checker was complaining about this. thanks > >> >>> + } else { >>> + for (i = 0; sctx && i < clone_sources_to_rollback; i++) >>> + btrfs_root_dec_send_in_progress( >>> + sctx->clone_roots[i].root); >> >> Same here. > > Notice there is a check above, > > sctx && i < clone_sources_to_rollback > > I really took care of that when i wrote the patch, please correct > me if i miss something here^_^. > > Thanks, > Wang >> >>> + /* >>> + * if we fail to add @send_root in clone roots, we still >>> + * need to decrease @send_in_progress count here. >>> + */ >>> + btrfs_root_dec_send_in_progress(send_root); >>> + } >>> if (sctx && !IS_ERR_OR_NULL(sctx->parent_root)) >>> btrfs_root_dec_send_in_progress(sctx->parent_root); >>> - btrfs_root_dec_send_in_progress(send_root); >>> >>> kfree(arg); >>> vfree(clone_sources_tmp); >>> -- >>> 1.8.3.1 >>> >>> -- >>> 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 >> >> >> >> -- >> Filipe David Manana, >> >> "Reasonable men adapt themselves to the world. >> Unreasonable men adapt the world to themselves. >> That's why all progress depends on unreasonable men." >> -- >> 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/send.c b/fs/btrfs/send.c index bff0b1a..83b6bdb 100644 --- a/fs/btrfs/send.c +++ b/fs/btrfs/send.c @@ -4752,6 +4752,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) u32 i; u64 *clone_sources_tmp = NULL; int clone_sources_to_rollback = 0; + int sort_clone_roots = 0; if (!capable(CAP_SYS_ADMIN)) return -EPERM; @@ -4942,6 +4943,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) sort(sctx->clone_roots, sctx->clone_roots_cnt, sizeof(*sctx->clone_roots), __clone_root_cmp_sort, NULL); + sort_clone_roots = 1; ret = send_subvol(sctx); if (ret < 0) @@ -4957,11 +4959,22 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) } out: - for (i = 0; sctx && i < clone_sources_to_rollback; i++) - btrfs_root_dec_send_in_progress(sctx->clone_roots[i].root); + if (sort_clone_roots) { + for (i = 0; i < sctx->clone_roots_cnt; i++) + btrfs_root_dec_send_in_progress( + sctx->clone_roots[i].root); + } else { + for (i = 0; sctx && i < clone_sources_to_rollback; i++) + btrfs_root_dec_send_in_progress( + sctx->clone_roots[i].root); + /* + * if we fail to add @send_root in clone roots, we still + * need to decrease @send_in_progress count here. + */ + btrfs_root_dec_send_in_progress(send_root); + } if (sctx && !IS_ERR_OR_NULL(sctx->parent_root)) btrfs_root_dec_send_in_progress(sctx->parent_root); - btrfs_root_dec_send_in_progress(send_root); kfree(arg); vfree(clone_sources_tmp);
Steps to reproduce: # mkfs.btrfs -f /dev/sda8 # mount /dev/sda8 /mnt # btrfs sub snapshot -r /mnt /mnt/snap1 # btrfs sub snapshot -r /mnt /mnt/snap2 # btrfs send /mnt/snap1 -p /mnt/snap2 -f /mnt/1 # dmesg The problem is that we will sort clone roots(include @send_root), it might push @send_root before thus @send_root's @send_in_progress will be decreased twice. Cc: David Sterba <dsterba@suse.cz> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> --- changelog v2->v3: add comments as david addressed. v1->v2: use right root to check --- fs/btrfs/send.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)