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@xxxxxx>
>>>
>>> Signed-off-by: Omar Sandoval <osandov@xxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
