| Message ID | 20180327070658.13064-5-lufq.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018年03月27日 15:06, Lu Fengqi wrote: > The function is used to determine whether the subvolume is intact. > > Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> > --- > Makefile | 3 ++- > undelete-subvol.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > undelete-subvol.h | 17 +++++++++++++++++ > 3 files changed, 72 insertions(+), 1 deletion(-) > create mode 100644 undelete-subvol.c > create mode 100644 undelete-subvol.h > > diff --git a/Makefile b/Makefile > index 6065522a615f..cb38984c7386 100644 > --- a/Makefile > +++ b/Makefile > @@ -116,7 +116,8 @@ objects = ctree.o disk-io.o kernel-lib/radix-tree.o extent-tree.o print-tree.o \ > qgroup.o free-space-cache.o kernel-lib/list_sort.o props.o \ > kernel-shared/ulist.o qgroup-verify.o backref.o string-table.o task-utils.o \ > inode.o file.o find-root.o free-space-tree.o help.o send-dump.o \ > - fsfeatures.o kernel-lib/tables.o kernel-lib/raid56.o transaction.o > + fsfeatures.o kernel-lib/tables.o kernel-lib/raid56.o transaction.o \ > + undelete-subvol.o > cmds_objects = cmds-subvolume.o cmds-filesystem.o cmds-device.o cmds-scrub.o \ > cmds-inspect.o cmds-balance.o cmds-send.o cmds-receive.o \ > cmds-quota.o cmds-qgroup.o cmds-replace.o check/main.o \ > diff --git a/undelete-subvol.c b/undelete-subvol.c > new file mode 100644 > index 000000000000..00fcc4895778 > --- /dev/null > +++ b/undelete-subvol.c > @@ -0,0 +1,53 @@ > +/* > + * Copyright (C) 2018 Fujitsu. All rights reserved. IIRC David will remove all such copy right line. Is there some principle about this, David? > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public > + * License v2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + */ > + > +#include "ctree.h" > + > +/* > + * Determines whether the subvolume is intact, according to the drop_progress > + * of the root item specified by subvol_id. > + * > + * Return true if the subvolume is intact, otherwise return false. > + */ > +static bool is_subvol_intact(struct btrfs_root *root, u64 subvol_id) Here we have 2 parameters which can represent a root, so please explain the meaning of each parameter. And after looking into the code, @root should be tree root, and in that case, I prefer passing @fs_info here to get rid of any confusion. > +{ > + struct btrfs_path path; > + struct btrfs_key key; > + struct extent_buffer *leaf; > + struct btrfs_root_item root_item; > + u64 offset; > + int ret; > + > + key.objectid = subvol_id; > + key.type = BTRFS_ROOT_ITEM_KEY; > + key.offset = 0; > + > + btrfs_init_path(&path); > + ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0); Instead of setting up keys and search manually, what about using btrfs_read_fs_root()? > + if (ret) { > + ret = false; > + goto out; > + } > + > + leaf = path.nodes[0]; > + offset = btrfs_item_ptr_offset(leaf, path.slots[0]); > + > + read_extent_buffer(leaf, &root_item, offset, sizeof(root_item)); > + > + /* the subvolume is intact if the objectid of drop_progress == 0 */ > + ret = btrfs_disk_key_objectid(&root_item.drop_progress) ? false : true; > + > +out: > + btrfs_release_path(&path); > + return ret; > +} > diff --git a/undelete-subvol.h b/undelete-subvol.h > new file mode 100644 > index 000000000000..7cfd100cce37 > --- /dev/null > +++ b/undelete-subvol.h > @@ -0,0 +1,17 @@ > +/* > + * Copyright (C) 2018 Fujitsu. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public > + * License v2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + */ > + > +#ifndef __BTRFS_UNDELETE_SUBVOLUME_H__ I think the empty header is not really needed here. What about introducing it when we have something here? Thanks, Qu > +#define __BTRFS_UNDELETE_SUBVOLUME_H__ > + > +#endif >
On Wed, Apr 18, 2018 at 01:12:10PM +0800, Qu Wenruo wrote: > > >On 2018年03月27日 15:06, Lu Fengqi wrote: >> The function is used to determine whether the subvolume is intact. >> >> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> >> --- >> Makefile | 3 ++- >> undelete-subvol.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> undelete-subvol.h | 17 +++++++++++++++++ >> 3 files changed, 72 insertions(+), 1 deletion(-) >> create mode 100644 undelete-subvol.c >> create mode 100644 undelete-subvol.h >> >> diff --git a/Makefile b/Makefile >> index 6065522a615f..cb38984c7386 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -116,7 +116,8 @@ objects = ctree.o disk-io.o kernel-lib/radix-tree.o extent-tree.o print-tree.o \ >> qgroup.o free-space-cache.o kernel-lib/list_sort.o props.o \ >> kernel-shared/ulist.o qgroup-verify.o backref.o string-table.o task-utils.o \ >> inode.o file.o find-root.o free-space-tree.o help.o send-dump.o \ >> - fsfeatures.o kernel-lib/tables.o kernel-lib/raid56.o transaction.o >> + fsfeatures.o kernel-lib/tables.o kernel-lib/raid56.o transaction.o \ >> + undelete-subvol.o >> cmds_objects = cmds-subvolume.o cmds-filesystem.o cmds-device.o cmds-scrub.o \ >> cmds-inspect.o cmds-balance.o cmds-send.o cmds-receive.o \ >> cmds-quota.o cmds-qgroup.o cmds-replace.o check/main.o \ >> diff --git a/undelete-subvol.c b/undelete-subvol.c >> new file mode 100644 >> index 000000000000..00fcc4895778 >> --- /dev/null >> +++ b/undelete-subvol.c >> @@ -0,0 +1,53 @@ >> +/* >> + * Copyright (C) 2018 Fujitsu. All rights reserved. > >IIRC David will remove all such copy right line. >Is there some principle about this, David? Are you referring to this patchset that replace GPL boilerplate by SPDX? https://patchwork.kernel.org/patch/10321621/ However, I haven't seen a similar patch in btrfs-progs. > >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public >> + * License v2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * General Public License for more details. >> + */ >> + >> +#include "ctree.h" >> + >> +/* >> + * Determines whether the subvolume is intact, according to the drop_progress >> + * of the root item specified by subvol_id. >> + * >> + * Return true if the subvolume is intact, otherwise return false. >> + */ >> +static bool is_subvol_intact(struct btrfs_root *root, u64 subvol_id) > >Here we have 2 parameters which can represent a root, so please explain >the meaning of each parameter. > >And after looking into the code, @root should be tree root, and in that >case, I prefer passing @fs_info here to get rid of any confusion. Make sense. > >> +{ >> + struct btrfs_path path; >> + struct btrfs_key key; >> + struct extent_buffer *leaf; >> + struct btrfs_root_item root_item; >> + u64 offset; >> + int ret; >> + >> + key.objectid = subvol_id; >> + key.type = BTRFS_ROOT_ITEM_KEY; >> + key.offset = 0; >> + >> + btrfs_init_path(&path); >> + ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0); > >Instead of setting up keys and search manually, what about using >btrfs_read_fs_root()? Indeed looks better. > >> + if (ret) { >> + ret = false; >> + goto out; >> + } >> + >> + leaf = path.nodes[0]; >> + offset = btrfs_item_ptr_offset(leaf, path.slots[0]); >> + >> + read_extent_buffer(leaf, &root_item, offset, sizeof(root_item)); >> + >> + /* the subvolume is intact if the objectid of drop_progress == 0 */ >> + ret = btrfs_disk_key_objectid(&root_item.drop_progress) ? false : true; >> + >> +out: >> + btrfs_release_path(&path); >> + return ret; >> +} >> diff --git a/undelete-subvol.h b/undelete-subvol.h >> new file mode 100644 >> index 000000000000..7cfd100cce37 >> --- /dev/null >> +++ b/undelete-subvol.h >> @@ -0,0 +1,17 @@ >> +/* >> + * Copyright (C) 2018 Fujitsu. All rights reserved. >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public >> + * License v2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * General Public License for more details. >> + */ >> + >> +#ifndef __BTRFS_UNDELETE_SUBVOLUME_H__ >I think the empty header is not really needed here. >What about introducing it when we have something here? That's OK. Thanks for your review. -- Thanks, Lu > >Thanks, >Qu > >> +#define __BTRFS_UNDELETE_SUBVOLUME_H__ >> + >> +#endif >> > -- 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 2018年05月07日 10:03, Lu Fengqi wrote: > On Wed, Apr 18, 2018 at 01:12:10PM +0800, Qu Wenruo wrote: >> >> >> On 2018年03月27日 15:06, Lu Fengqi wrote: >>> The function is used to determine whether the subvolume is intact. >>> >>> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> >>> --- >>> Makefile | 3 ++- >>> undelete-subvol.c | 53 >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> undelete-subvol.h | 17 +++++++++++++++++ >>> 3 files changed, 72 insertions(+), 1 deletion(-) >>> create mode 100644 undelete-subvol.c >>> create mode 100644 undelete-subvol.h >>> >>> diff --git a/Makefile b/Makefile >>> index 6065522a615f..cb38984c7386 100644 >>> --- a/Makefile >>> +++ b/Makefile >>> @@ -116,7 +116,8 @@ objects =tree.o disk-io.o kernel-lib/radix-tree.o >>> extent-tree.o print-tree.o \ >>> qgroup.o free-space-cache.o kernel-lib/list_sort.o props.o \ >>> kernel-shared/ulist.o qgroup-verify.o backref.o string-table.o >>> task-utils.o \ >>> inode.o file.o find-root.o free-space-tree.o help.o send-dump.o \ >>> - fsfeatures.o kernel-lib/tables.o kernel-lib/raid56.o >>> transaction.o >>> + fsfeatures.o kernel-lib/tables.o kernel-lib/raid56.o >>> transaction.o \ >>> + undelete-subvol.o >>> cmds_objects =mds-subvolume.o cmds-filesystem.o cmds-device.o >>> cmds-scrub.o \ >>> cmds-inspect.o cmds-balance.o cmds-send.o cmds-receive.o \ >>> cmds-quota.o cmds-qgroup.o cmds-replace.o check/main.o \ >>> diff --git a/undelete-subvol.c b/undelete-subvol.c >>> new file mode 100644 >>> index 000000000000..00fcc4895778 >>> --- /dev/null >>> +++ b/undelete-subvol.c >>> @@ -0,0 +1,53 @@ >>> +/* >>> + * Copyright (C) 2018 Fujitsu. All rights reserved. >> >> IIRC David will remove all such copy right line. >> Is there some principle about this, David? > > Are you referring to this patchset that replace GPL boilerplate by SPDX? > https://patchwork.kernel.org/patch/10321621/ > > However, I haven't seen a similar patch in btrfs-progs. > Nope, I mean David will remove the Copyright (C) line when applying. Although I'm not completely sure. Thanks, Qu >> >>> + * >>> + * This program is free software; you can redistribute it and/or >>> + * modify it under the terms of the GNU General Public >>> + * License v2 as published by the Free Software Foundation. >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>> + * General Public License for more details. >>> + */ >>> + >>> +#include "ctree.h" >>> + >>> +/* >>> + * Determines whether the subvolume is intact, according to the >>> drop_progress >>> + * of the root item specified by subvol_id. >>> + * >>> + * Return true if the subvolume is intact, otherwise return false. >>> + */ >>> +static bool is_subvol_intact(struct btrfs_root *root, u64 subvol_id) >> >> Here we have 2 parameters which can represent a root, so please explain >> the meaning of each parameter. >> >> And after looking into the code, @root should be tree root, and in that >> case, I prefer passing @fs_info here to get rid of any confusion. > > Make sense. > >> >>> +{ >>> + struct btrfs_path path; >>> + struct btrfs_key key; >>> + struct extent_buffer *leaf; >>> + struct btrfs_root_item root_item; >>> + u64 offset; >>> + int ret; >>> + >>> + key.objectid =ubvol_id; >>> + key.type =TRFS_ROOT_ITEM_KEY; >>> + key.offset =; >>> + >>> + btrfs_init_path(&path); >>> + ret =trfs_search_slot(NULL, root, &key, &path, 0, 0); >> >> Instead of setting up keys and search manually, what about using >> btrfs_read_fs_root()? > > Indeed looks better. > >> >>> + if (ret) { >>> + ret =alse; >>> + goto out; >>> + } >>> + >>> + leaf =ath.nodes[0]; >>> + offset =trfs_item_ptr_offset(leaf, path.slots[0]); >>> + >>> + read_extent_buffer(leaf, &root_item, offset, sizeof(root_item)); >>> + >>> + /* the subvolume is intact if the objectid of drop_progress =0 */ >>> + ret =trfs_disk_key_objectid(&root_item.drop_progress) ? false : >>> true; >>> + >>> +out: >>> + btrfs_release_path(&path); >>> + return ret; >>> +} >>> diff --git a/undelete-subvol.h b/undelete-subvol.h >>> new file mode 100644 >>> index 000000000000..7cfd100cce37 >>> --- /dev/null >>> +++ b/undelete-subvol.h >>> @@ -0,0 +1,17 @@ >>> +/* >>> + * Copyright (C) 2018 Fujitsu. All rights reserved. >>> + * >>> + * This program is free software; you can redistribute it and/or >>> + * modify it under the terms of the GNU General Public >>> + * License v2 as published by the Free Software Foundation. >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>> + * General Public License for more details. >>> + */ >>> + >>> +#ifndef __BTRFS_UNDELETE_SUBVOLUME_H__ >> I think the empty header is not really needed here. >> What about introducing it when we have something here? > > That's OK. > > Thanks for your review. > > -- > Thanks, > Lu > >> >> Thanks, >> Qu >> >>> +#define __BTRFS_UNDELETE_SUBVOLUME_H__ >>> + >>> +#endif >>> >> > > > > > > -- > 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 Mon, May 07, 2018 at 10:20:57AM +0800, Qu Wenruo wrote: > >>> +++ b/undelete-subvol.c > >>> @@ -0,0 +1,53 @@ > >>> +/* > >>> + * Copyright (C) 2018 Fujitsu. All rights reserved. > >> > >> IIRC David will remove all such copy right line. > >> Is there some principle about this, David? > > > > Are you referring to this patchset that replace GPL boilerplate by SPDX? > > https://patchwork.kernel.org/patch/10321621/ > > > > However, I haven't seen a similar patch in btrfs-progs. > > > > Nope, I mean David will remove the Copyright (C) line when applying. > Although I'm not completely sure. Removing copyright notices should not be done without an ack from all parties and I don't remove the copyright notices from patches if present. I personally do not understand and see the point of the explicit mentions if we have the signed-off by in git history. Also, when the file is touched by many different people over time, the line 'copyright by Evil Company' looks like all the credit goes to the single entitiy. There's a point if the code is copied from another source, like the test/sha* code that's from an RFC. So, I decided to not care too much about the copyright line as long as there's a signed-off and authorship is GPL compatible. -- 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 2018年05月07日 19:40, David Sterba wrote: > On Mon, May 07, 2018 at 10:20:57AM +0800, Qu Wenruo wrote: >>>>> +++ b/undelete-subvol.c >>>>> @@ -0,0 +1,53 @@ >>>>> +/* >>>>> + * Copyright (C) 2018 Fujitsu. All rights reserved. >>>> >>>> IIRC David will remove all such copy right line. >>>> Is there some principle about this, David? >>> >>> Are you referring to this patchset that replace GPL boilerplate by SPDX? >>> https://patchwork.kernel.org/patch/10321621/ >>> >>> However, I haven't seen a similar patch in btrfs-progs. >>> >> >> Nope, I mean David will remove the Copyright (C) line when applying. >> Although I'm not completely sure. > > Removing copyright notices should not be done without an ack from all > parties and I don't remove the copyright notices from patches if > present. > > I personally do not understand and see the point of the explicit > mentions if we have the signed-off by in git history. Also, when the > file is touched by many different people over time, the line 'copyright > by Evil Company' looks like all the credit goes to the single entitiy. That seems to be the point. :) > > There's a point if the code is copied from another source, like the > test/sha* code that's from an RFC. > > So, I decided to not care too much about the copyright line as long as > there's a signed-off and authorship is GPL compatible. Glad that the policy is now clear. Thanks, Qu > -- > 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/Makefile b/Makefile index 6065522a615f..cb38984c7386 100644 --- a/Makefile +++ b/Makefile @@ -116,7 +116,8 @@ objects = ctree.o disk-io.o kernel-lib/radix-tree.o extent-tree.o print-tree.o \ qgroup.o free-space-cache.o kernel-lib/list_sort.o props.o \ kernel-shared/ulist.o qgroup-verify.o backref.o string-table.o task-utils.o \ inode.o file.o find-root.o free-space-tree.o help.o send-dump.o \ - fsfeatures.o kernel-lib/tables.o kernel-lib/raid56.o transaction.o + fsfeatures.o kernel-lib/tables.o kernel-lib/raid56.o transaction.o \ + undelete-subvol.o cmds_objects = cmds-subvolume.o cmds-filesystem.o cmds-device.o cmds-scrub.o \ cmds-inspect.o cmds-balance.o cmds-send.o cmds-receive.o \ cmds-quota.o cmds-qgroup.o cmds-replace.o check/main.o \ diff --git a/undelete-subvol.c b/undelete-subvol.c new file mode 100644 index 000000000000..00fcc4895778 --- /dev/null +++ b/undelete-subvol.c @@ -0,0 +1,53 @@ +/* + * Copyright (C) 2018 Fujitsu. All rights reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public + * License v2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + */ + +#include "ctree.h" + +/* + * Determines whether the subvolume is intact, according to the drop_progress + * of the root item specified by subvol_id. + * + * Return true if the subvolume is intact, otherwise return false. + */ +static bool is_subvol_intact(struct btrfs_root *root, u64 subvol_id) +{ + struct btrfs_path path; + struct btrfs_key key; + struct extent_buffer *leaf; + struct btrfs_root_item root_item; + u64 offset; + int ret; + + key.objectid = subvol_id; + key.type = BTRFS_ROOT_ITEM_KEY; + key.offset = 0; + + btrfs_init_path(&path); + ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0); + if (ret) { + ret = false; + goto out; + } + + leaf = path.nodes[0]; + offset = btrfs_item_ptr_offset(leaf, path.slots[0]); + + read_extent_buffer(leaf, &root_item, offset, sizeof(root_item)); + + /* the subvolume is intact if the objectid of drop_progress == 0 */ + ret = btrfs_disk_key_objectid(&root_item.drop_progress) ? false : true; + +out: + btrfs_release_path(&path); + return ret; +} diff --git a/undelete-subvol.h b/undelete-subvol.h new file mode 100644 index 000000000000..7cfd100cce37 --- /dev/null +++ b/undelete-subvol.h @@ -0,0 +1,17 @@ +/* + * Copyright (C) 2018 Fujitsu. All rights reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public + * License v2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + */ + +#ifndef __BTRFS_UNDELETE_SUBVOLUME_H__ +#define __BTRFS_UNDELETE_SUBVOLUME_H__ + +#endif
The function is used to determine whether the subvolume is intact. Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> --- Makefile | 3 ++- undelete-subvol.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ undelete-subvol.h | 17 +++++++++++++++++ 3 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 undelete-subvol.c create mode 100644 undelete-subvol.h