[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-btrfs
Subject: Re: [PATCH v2 14/27] libbtrfsutil: add btrfs_util_deleted_subvolumes()
From: "Misono, Tomohiro" <misono.tomohiro () jp ! fujitsu ! com>
Date: 2018-02-28 4:11:06
Message-ID: e4d08b0c-7438-32d8-e076-90fc4c4f6c79 () jp ! fujitsu ! com
[Download RAW message or body]
On 2018/02/24 8:33, Omar Sandoval wrote:
> On Fri, Feb 23, 2018 at 11:12:56AM +0900, Misono, Tomohiro wrote:
> >
> > On 2018/02/16 4:04, Omar Sandoval wrote:
> > > From: Omar Sandoval <osandov@fb.com>
> > >
> > > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > > ---
> > > libbtrfsutil/btrfsutil.h | 21 +++++++
> > > libbtrfsutil/python/btrfsutilpy.h | 3 +
> > > libbtrfsutil/python/module.c | 30 ++++++++++
> > > libbtrfsutil/python/qgroup.c | 17 +-----
> > > libbtrfsutil/python/subvolume.c | 30 ++++++++++
> > > libbtrfsutil/python/tests/test_subvolume.py | 8 +++
> > > libbtrfsutil/subvolume.c | 89 +++++++++++++++++++++++++++++
> > > 7 files changed, 183 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/libbtrfsutil/btrfsutil.h b/libbtrfsutil/btrfsutil.h
> > > index 00c86174..677ab3c1 100644
> > > --- a/libbtrfsutil/btrfsutil.h
> > > +++ b/libbtrfsutil/btrfsutil.h
> > > @@ -534,6 +534,27 @@ enum btrfs_util_error \
> > > btrfs_util_subvolume_iterator_next_info(struct btrfs_util_ char **path_ret,
> > > struct btrfs_util_subvolume_info *subvol);
> > >
> > > +/**
> > > + * btrfs_util_deleted_subvolumes() - Get a list of subvolume which have been
> > > + * deleted but not yet cleaned up.
> > > + * @path: Path on a Btrfs filesystem.
> > > + * @ids: Returned array of subvolume IDs.
> > > + * @n: Returned number of IDs in the @ids array.
> > > + *
> > > + * This requires appropriate privilege (CAP_SYS_ADMIN).
> > > + *
> > > + * Return: %BTRFS_UTIL_OK on success, non-zero error code on failure.
> > > + */
> > > +enum btrfs_util_error btrfs_util_deleted_subvolumes(const char *path,
> > > + uint64_t **ids,
> > > + size_t *n);
> > > +
> > > +/**
> > > + * btrfs_util_deleted_subvolumes_fd() - See btrfs_util_deleted_subvolumes().
> > > + */
> > > +enum btrfs_util_error btrfs_util_deleted_subvolumes_fd(int fd, uint64_t **ids,
> > > + size_t *n);
> > > +
> > > /**
> > > * btrfs_util_create_qgroup_inherit() - Create a qgroup inheritance specifier
> > > * for btrfs_util_create_subvolume() or btrfs_util_create_snapshot().
> > > diff --git a/libbtrfsutil/python/btrfsutilpy.h \
> > > b/libbtrfsutil/python/btrfsutilpy.h index b3ec047f..be5122e2 100644
> > > --- a/libbtrfsutil/python/btrfsutilpy.h
> > > +++ b/libbtrfsutil/python/btrfsutilpy.h
> > > @@ -54,6 +54,8 @@ struct path_arg {
> > > int path_converter(PyObject *o, void *p);
> > > void path_cleanup(struct path_arg *path);
> > >
> > > +PyObject *list_from_uint64_array(const uint64_t *arr, size_t n);
> > > +
> > > void SetFromBtrfsUtilError(enum btrfs_util_error err);
> > > void SetFromBtrfsUtilErrorWithPath(enum btrfs_util_error err,
> > > struct path_arg *path);
> > > @@ -72,6 +74,7 @@ PyObject *set_default_subvolume(PyObject *self, PyObject \
> > > *args, PyObject *kwds); PyObject *create_subvolume(PyObject *self, PyObject \
> > > *args, PyObject *kwds); PyObject *create_snapshot(PyObject *self, PyObject \
> > > *args, PyObject *kwds); PyObject *delete_subvolume(PyObject *self, PyObject \
> > > *args, PyObject *kwds); +PyObject *deleted_subvolumes(PyObject *self, PyObject \
> > > *args, PyObject *kwds);
> > > void add_module_constants(PyObject *m);
> > >
> > > diff --git a/libbtrfsutil/python/module.c b/libbtrfsutil/python/module.c
> > > index e995a1be..eaa062ac 100644
> > > --- a/libbtrfsutil/python/module.c
> > > +++ b/libbtrfsutil/python/module.c
> > > @@ -125,6 +125,29 @@ err:
> > > return 0;
> > > }
> > >
> > > +PyObject *list_from_uint64_array(const uint64_t *arr, size_t n)
> > > +{
> > > + PyObject *ret;
> > > + size_t i;
> > > +
> > > + ret = PyList_New(n);
> > > + if (!ret)
> > > + return NULL;
> > > +
> > > + for (i = 0; i < n; i++) {
> > > + PyObject *tmp;
> > > +
> > > + tmp = PyLong_FromUnsignedLongLong(arr[i]);
> > > + if (!tmp) {
> > > + Py_DECREF(ret);
> > > + return NULL;
> > > + }
> > > + PyList_SET_ITEM(ret, i, tmp);
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > void path_cleanup(struct path_arg *path)
> > > {
> > > Py_CLEAR(path->object);
> > > @@ -214,6 +237,13 @@ static PyMethodDef btrfsutil_methods[] = {
> > > "path -- string, bytes, or path-like object\n"
> > > "recursive -- if the given subvolume has child subvolumes, delete\n"
> > > "them instead of failing"},
> > > + {"deleted_subvolumes", (PyCFunction)deleted_subvolumes,
> > > + METH_VARARGS | METH_KEYWORDS,
> > > + "deleted_subvolumes(path)\n\n"
> > > + "Get the list of subvolume IDs which have been deleted but not yet\n"
> > > + "cleaned up\n\n"
> > > + "Arguments:\n"
> > > + "path -- string, bytes, path-like object, or open file descriptor"},
> > > {},
> > > };
> > >
> > > diff --git a/libbtrfsutil/python/qgroup.c b/libbtrfsutil/python/qgroup.c
> > > index 69716d92..44ac5ebc 100644
> > > --- a/libbtrfsutil/python/qgroup.c
> > > +++ b/libbtrfsutil/python/qgroup.c
> > > @@ -55,25 +55,12 @@ static PyObject *QgroupInherit_getattro(QgroupInherit \
> > > *self, PyObject *nameobj) }
> > >
> > > if (strcmp(name, "groups") == 0) {
> > > - PyObject *ret, *tmp;
> > > const uint64_t *arr;
> > > - size_t n, i;
> > > + size_t n;
> > >
> > > btrfs_util_qgroup_inherit_get_groups(self->inherit, &arr, &n);
> > > - ret = PyList_New(n);
> > > - if (!ret)
> > > - return NULL;
> > > -
> > > - for (i = 0; i < n; i++) {
> > > - tmp = PyLong_FromUnsignedLongLong(arr[i]);
> > > - if (!tmp) {
> > > - Py_DECREF(ret);
> > > - return NULL;
> > > - }
> > > - PyList_SET_ITEM(ret, i, tmp);
> > > - }
> > >
> > > - return ret;
> > > + return list_from_uint64_array(arr, n);
> > > } else {
> > > return PyObject_GenericGetAttr((PyObject *)self, nameobj);
> > > }
> > > diff --git a/libbtrfsutil/python/subvolume.c b/libbtrfsutil/python/subvolume.c
> > > index eb3f6e27..069e606b 100644
> > > --- a/libbtrfsutil/python/subvolume.c
> > > +++ b/libbtrfsutil/python/subvolume.c
> > > @@ -425,6 +425,36 @@ PyObject *delete_subvolume(PyObject *self, PyObject *args, \
> > > PyObject *kwds) Py_RETURN_NONE;
> > > }
> > >
> > > +PyObject *deleted_subvolumes(PyObject *self, PyObject *args, PyObject *kwds)
> > > +{
> > > + static char *keywords[] = {"path", NULL};
> > > + struct path_arg path = {.allow_fd = true};
> > > + PyObject *ret;
> > > + uint64_t *ids;
> > > + size_t n;
> > > + enum btrfs_util_error err;
> > > +
> > > + if (!PyArg_ParseTupleAndKeywords(args, kwds, "O&:deleted_subvolumes",
> > > + keywords, &path_converter, &path))
> > > + return NULL;
> > > +
> > > + if (path.path)
> > > + err = btrfs_util_deleted_subvolumes(path.path, &ids, &n);
> > > + else
> > > + err = btrfs_util_deleted_subvolumes_fd(path.fd, &ids, &n);
> > > + if (err) {
> > > + SetFromBtrfsUtilErrorWithPath(err, &path);
> > > + path_cleanup(&path);
> > > + return NULL;
> > > + }
> > > +
> > > + path_cleanup(&path);
> > > +
> > > + ret = list_from_uint64_array(ids, n);
> > > + free(ids);
> > > + return ret;
> > > +}
> > > +
> > > typedef struct {
> > > PyObject_HEAD
> > > struct btrfs_util_subvolume_iterator *iter;
> > > diff --git a/libbtrfsutil/python/tests/test_subvolume.py \
> > > b/libbtrfsutil/python/tests/test_subvolume.py index 08083abe..a46d4a34 100644
> > > --- a/libbtrfsutil/python/tests/test_subvolume.py
> > > +++ b/libbtrfsutil/python/tests/test_subvolume.py
> > > @@ -318,6 +318,14 @@ class TestSubvolume(BtrfsTestCase):
> > > btrfsutil.delete_subvolume(subvol + '5', recursive=True)
> > > self.assertFalse(os.path.exists(subvol + '5'))
> > >
> > > + def test_deleted_subvolumes(self):
> > > + subvol = os.path.join(self.mountpoint, 'subvol')
> > > + btrfsutil.create_subvolume(subvol + '1')
> > > + btrfsutil.delete_subvolume(subvol + '1')
> > > + for arg in self.path_or_fd(self.mountpoint):
> > > + with self.subTest(type=type(arg)):
> > > + self.assertEqual(btrfsutil.deleted_subvolumes(arg), [256])
> > > +
> > > def test_subvolume_iterator(self):
> > > pwd = os.getcwd()
> > > try:
> > > diff --git a/libbtrfsutil/subvolume.c b/libbtrfsutil/subvolume.c
> > > index 908e71db..4ae581b2 100644
> > > --- a/libbtrfsutil/subvolume.c
> > > +++ b/libbtrfsutil/subvolume.c
> > > @@ -1277,3 +1277,92 @@ PUBLIC enum btrfs_util_error \
> > > btrfs_util_subvolume_iterator_next_info(struct btrf
> > > return btrfs_util_subvolume_info_fd(iter->fd, id, subvol);
> > > }
> > > +
> > > +PUBLIC enum btrfs_util_error btrfs_util_deleted_subvolumes(const char *path,
> > > + uint64_t **ids,
> > > + size_t *n)
> > > +{
> > > + enum btrfs_util_error err;
> > > + int fd;
> > > +
> > > + fd = open(path, O_RDONLY);
> > > + if (fd == -1)
> > > + return BTRFS_UTIL_ERROR_OPEN_FAILED;
> > > +
> > > + err = btrfs_util_deleted_subvolumes_fd(fd, ids, n);
> > > + SAVE_ERRNO_AND_CLOSE(fd);
> > > + return err;
> > > +}
> > > +
> > > +PUBLIC enum btrfs_util_error btrfs_util_deleted_subvolumes_fd(int fd,
> > > + uint64_t **ids,
> > > + size_t *n)
> > > +{
> > > + size_t capacity = 0;
> > > + struct btrfs_ioctl_search_args search = {
> > > + .key = {
> > > + .tree_id = BTRFS_ROOT_TREE_OBJECTID,
> > > + .min_objectid = BTRFS_ORPHAN_OBJECTID,
> > > + .max_objectid = BTRFS_ORPHAN_OBJECTID,
> > > + .min_type = BTRFS_ORPHAN_ITEM_KEY,
> > > + .max_type = BTRFS_ORPHAN_ITEM_KEY,
> > > + .min_offset = 0,
> > > + .max_offset = UINT64_MAX,
> > > + .min_transid = 0,
> > > + .max_transid = UINT64_MAX,
> > > + .nr_items = 0,
> > > + },
> > > + };
> > It seems btrfs_util_deleted_subvolume() does not works perfectly.
> >
> > Above search will return all ORPHAN_ITEM but some may be of free space cache \
> > inode (correct me if I'm wrong). Since this is a public function, we should \
> > filter those non-subvolume objectid by checking root tree again. I think this is \
> > the reason of failure of misc-test/013 after merging 26th patch.
>
> So looking at the code that this replaces, I don't see the difference:
>
> static int enumerate_dead_subvols(int fd, u64 **ids)
> {
> int ret;
> struct btrfs_ioctl_search_args args;
> struct btrfs_ioctl_search_key *sk = &args.key;
> int idx = 0;
> int count = 0;
>
> memset(&args, 0, sizeof(args));
>
> sk->tree_id = BTRFS_ROOT_TREE_OBJECTID;
> sk->min_objectid = BTRFS_ORPHAN_OBJECTID;
> sk->max_objectid = BTRFS_ORPHAN_OBJECTID;
> sk->min_type = BTRFS_ORPHAN_ITEM_KEY;
> sk->max_type = BTRFS_ORPHAN_ITEM_KEY;
> sk->min_offset = 0;
> sk->max_offset = (u64)-1;
> sk->min_transid = 0;
> sk->max_transid = (u64)-1;
> sk->nr_items = 4096;
>
> *ids = NULL;
> while (1) {
> struct btrfs_ioctl_search_header *sh;
> unsigned long off;
> int i;
>
> ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
> if (ret < 0)
> return -errno;
>
> if (!sk->nr_items)
> return idx;
>
> off = 0;
> for (i = 0; i < sk->nr_items; i++) {
> sh = (struct btrfs_ioctl_search_header*)(args.buf + off);
> off += sizeof(*sh);
>
> if (btrfs_search_header_type(sh)
> == BTRFS_ORPHAN_ITEM_KEY) {
> if (idx >= count) {
> u64 *newids;
>
> count += SUBVOL_ID_BATCH;
> newids = (u64*)realloc(*ids,
> count * sizeof(u64));
> if (!newids)
> return -ENOMEM;
> *ids = newids;
> }
> (*ids)[idx] = btrfs_search_header_offset(sh);
> idx++;
> }
> off += btrfs_search_header_len(sh);
>
> sk->min_objectid = btrfs_search_header_objectid(sh);
> sk->min_type = btrfs_search_header_type(sh);
> sk->min_offset = btrfs_search_header_offset(sh);
> }
> if (sk->min_offset < (u64)-1)
> sk->min_offset++;
> else
> break;
> if (sk->min_type != BTRFS_ORPHAN_ITEM_KEY)
> break;
> if (sk->min_objectid != BTRFS_ORPHAN_OBJECTID)
> break;
> }
>
> return idx;
> }
>
> This function does the same exact search, as far as I can tell. What does the
> test failure look like? I'll try to reproduce it on my side.
>
Hello, sorry for late replay.
Actually this is not a problem for functionality of "sub list -d" or "sub sync",
because after this function is called we check root tree to see if ROOT_ITEM
for deleted subvolume still exists or not. If the returned id is not related \
subvolume, there is no ROOT_ITEM at first.
Since your github branch (libbtrfs) is updated to skip \
BTRFS_UTIL_ERROR_SUBVOLUME_NOT_FOUND as I said in 26th patch, there is no test \
failure anymore.
But if I run misc-test/013 (on 4.16-rc3) with following diff[1] on libbtrfs branch to \
show which objectid returns BTRFS_UTIL_ERROR_SUBVOLUME_NOT_FOUND after \
btrfs_util_subvolume_info_fd(), I can see the garbage id in \
tests/misc-tests-result.txt: ===
[snip]
ID 291 gen 78 top level 5 path snap34
ID 293 gen 78 top level 5 path snap35
[snip]
Subvolume id 291 is gone
Subvolume id 293 is gone
[snip]
############### /home/misono/src/btrfs-progs/btrfs subvolum list -d .
cleaned: 292 <- there was no subvolume with objectid 292
[snip]
===
I'm not sure where this number comes from, but isn't it better to filter them?
---
cmds-subvolume.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 5fa390ff..b034bb77 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -1112,6 +1112,7 @@ static struct subvol_list *btrfs_list_deleted_subvols(int fd,
* The subvolume might have been cleaned up since it was
* returned.
*/
+ printf("cleaned: %ld\n", ids[i]);
continue;
} else if (err) {
error_btrfs_util(err);
---
--
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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic