From dd64132044e0387f9cdef62b995b88db62f124be Mon Sep 17 00:00:00 2001 From: VinayBhaskar-V Date: Tue, 26 Nov 2024 16:48:51 +0530 Subject: [PATCH 1/2] librbd: add rbd_diff_iterate3() API to take source snapshot by ID Allow a diff to start from a non-user snapshot. This would be used by "rbd du" command to account for non-user snapshots which are currently just skipped potentially resulting in underreported space usage and in other places. Fixes: https://tracker.ceph.com/issues/65720 Co-authored-by: Ilya Dryomov Signed-off-by: Vinay Bhaskar Varada Signed-off-by: Ilya Dryomov (cherry picked from commit 54f47cc28ffd2d29b4f8cfaf56a5a5be2909bde7) Conflicts: src/include/rbd/librbd.h [ commit e5ccce14c4b0 ("rbd: add group snap info command") not in reef ] src/test/pybind/test_rbd.py [ commit d7fd66ec9944 ("librbd: add rbd_clone4() API to take parent snapshot by ID") not in reef ] --- src/include/rbd/librbd.h | 14 +++ src/include/rbd/librbd.hpp | 3 + src/librbd/api/DiffIterate.cc | 28 ++--- src/librbd/api/DiffIterate.h | 16 +-- src/librbd/api/Trash.cc | 3 +- src/librbd/librbd.cc | 83 +++++++++++--- src/pybind/rbd/c_rbd.pxd | 8 ++ src/pybind/rbd/mock_rbd.pxi | 9 ++ src/pybind/rbd/rbd.pyx | 48 ++++++-- src/test/librbd/test_internal.cc | 3 +- src/test/pybind/test_rbd.py | 181 +++++++++++++++++++++++++++++-- 11 files changed, 331 insertions(+), 65 deletions(-) diff --git a/src/include/rbd/librbd.h b/src/include/rbd/librbd.h index 7ae20e4dd58..3a58a46064a 100644 --- a/src/include/rbd/librbd.h +++ b/src/include/rbd/librbd.h @@ -51,6 +51,7 @@ extern "C" { #define LIBRBD_SUPPORTS_WRITE_ZEROES 1 #define LIBRBD_SUPPORTS_ENCRYPTION 1 #define LIBRBD_SUPPORTS_ENCRYPTION_LOAD2 1 +#define LIBRBD_SUPPORTS_DIFF_ITERATE3 1 #if __GNUC__ >= 4 #define CEPH_RBD_API __attribute__ ((visibility ("default"))) @@ -375,6 +376,14 @@ enum { RBD_WRITE_ZEROES_FLAG_THICK_PROVISION = (1U<<0), /* fully allocated zeroed extent */ }; +/* rbd_diff_iterate3 flags */ +enum { + /* full history diff should include parent */ + RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT = (1U<<0), + /* diff extents should cover whole object */ + RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT = (1U<<1), +}; + typedef enum { RBD_ENCRYPTION_FORMAT_LUKS1 = 0, RBD_ENCRYPTION_FORMAT_LUKS2 = 1, @@ -1189,6 +1198,11 @@ CEPH_RBD_API int rbd_diff_iterate2(rbd_image_t image, uint8_t include_parent, uint8_t whole_object, int (*cb)(uint64_t, size_t, int, void *), void *arg); +CEPH_RBD_API int rbd_diff_iterate3(rbd_image_t image, uint64_t from_snap_id, + uint64_t ofs, uint64_t len, uint32_t flags, + int (*cb)(uint64_t, size_t, int, void *), + void *arg); + CEPH_RBD_API ssize_t rbd_write(rbd_image_t image, uint64_t ofs, size_t len, const char *buf); /* diff --git a/src/include/rbd/librbd.hpp b/src/include/rbd/librbd.hpp index 5d307cdedf5..a99bc048a1d 100644 --- a/src/include/rbd/librbd.hpp +++ b/src/include/rbd/librbd.hpp @@ -707,6 +707,9 @@ public: uint64_t ofs, uint64_t len, bool include_parent, bool whole_object, int (*cb)(uint64_t, size_t, int, void *), void *arg); + int diff_iterate3(uint64_t from_snap_id, + uint64_t ofs, uint64_t len, uint32_t flags, + int (*cb)(uint64_t, size_t, int, void *), void *arg); ssize_t write(uint64_t ofs, size_t len, ceph::bufferlist& bl); /* @param op_flags see librados.h constants beginning with LIBRADOS_OP_FLAG */ diff --git a/src/librbd/api/DiffIterate.cc b/src/librbd/api/DiffIterate.cc index f7dd57504db..03d27354953 100644 --- a/src/librbd/api/DiffIterate.cc +++ b/src/librbd/api/DiffIterate.cc @@ -195,15 +195,12 @@ int simple_diff_cb(uint64_t off, size_t len, int exists, void *arg) { } // anonymous namespace template -int DiffIterate::diff_iterate(I *ictx, - const cls::rbd::SnapshotNamespace& from_snap_namespace, - const char *fromsnapname, +int DiffIterate::diff_iterate(I *ictx, uint64_t from_snap_id, uint64_t off, uint64_t len, bool include_parent, bool whole_object, int (*cb)(uint64_t, size_t, int, void *), void *arg) { - ldout(ictx->cct, 10) << "from_snap_namespace=" << from_snap_namespace - << ", fromsnapname=" << (fromsnapname ?: "") + ldout(ictx->cct, 10) << "from_snap_id=" << from_snap_id << ", off=" << off << ", len=" << len << ", include_parent=" << include_parent @@ -249,7 +246,7 @@ int DiffIterate::diff_iterate(I *ictx, // lock is acquired // acquire exclusive lock only if not busy (i.e. don't request), // throttle acquisition attempts and ignore errors - if (fromsnapname == nullptr && whole_object && + if (from_snap_id == 0 && whole_object && should_try_acquire_lock(ictx)) { C_SaferCond lock_ctx; ictx->exclusive_lock->try_acquire_lock(&lock_ctx); @@ -259,7 +256,7 @@ int DiffIterate::diff_iterate(I *ictx, } } - DiffIterate command(*ictx, from_snap_namespace, fromsnapname, off, len, + DiffIterate command(*ictx, from_snap_id, off, len, include_parent, whole_object, cb, arg); r = command.execute(); return r; @@ -295,24 +292,23 @@ int DiffIterate::execute() { ceph_assert(m_image_ctx.data_ctx.is_valid()); - librados::snap_t from_snap_id = 0; + librados::snap_t from_snap_id = m_from_snap_id; librados::snap_t end_snap_id; uint64_t from_size = 0; uint64_t end_size; { std::shared_lock image_locker{m_image_ctx.image_lock}; - if (m_from_snap_name) { - from_snap_id = m_image_ctx.get_snap_id(m_from_snap_namespace, - m_from_snap_name); - from_size = m_image_ctx.get_image_size(from_snap_id); + if (from_snap_id != 0) { + auto info = m_image_ctx.get_snap_info(from_snap_id); + if (info == nullptr) { + return -ENOENT; + } + from_size = info->size; } end_snap_id = m_image_ctx.snap_id; end_size = m_image_ctx.get_image_size(end_snap_id); } - if (from_snap_id == CEPH_NOSNAP) { - return -ENOENT; - } if (from_snap_id > end_snap_id) { return -EINVAL; } @@ -352,7 +348,7 @@ int DiffIterate::execute() { if (m_image_ctx.prune_parent_extents(parent_extents, io::ImageArea::DATA, raw_overlap, false) > 0) { ldout(cct, 10) << " first getting parent diff" << dendl; - DiffIterate diff_parent(*m_image_ctx.parent, {}, nullptr, + DiffIterate diff_parent(*m_image_ctx.parent, 0, parent_extents[0].first, parent_extents[0].second, true, true, &simple_diff_cb, &parent_diff); diff --git a/src/librbd/api/DiffIterate.h b/src/librbd/api/DiffIterate.h index c53b0e995b6..d19d1b3dae3 100644 --- a/src/librbd/api/DiffIterate.h +++ b/src/librbd/api/DiffIterate.h @@ -20,9 +20,7 @@ class DiffIterate { public: typedef int (*Callback)(uint64_t, size_t, int, void *); - static int diff_iterate(ImageCtxT *ictx, - const cls::rbd::SnapshotNamespace& from_snap_namespace, - const char *fromsnapname, + static int diff_iterate(ImageCtxT *ictx, uint64_t from_snap_id, uint64_t off, uint64_t len, bool include_parent, bool whole_object, int (*cb)(uint64_t, size_t, int, void *), @@ -30,8 +28,7 @@ public: private: ImageCtxT &m_image_ctx; - cls::rbd::SnapshotNamespace m_from_snap_namespace; - const char* m_from_snap_name; + uint64_t m_from_snap_id; uint64_t m_offset; uint64_t m_length; bool m_include_parent; @@ -39,13 +36,12 @@ private: Callback m_callback; void *m_callback_arg; - DiffIterate(ImageCtxT &image_ctx, - const cls::rbd::SnapshotNamespace& from_snap_namespace, - const char *from_snap_name, uint64_t off, uint64_t len, + DiffIterate(ImageCtxT &image_ctx, uint64_t from_snap_id, + uint64_t off, uint64_t len, bool include_parent, bool whole_object, Callback callback, void *callback_arg) - : m_image_ctx(image_ctx), m_from_snap_namespace(from_snap_namespace), - m_from_snap_name(from_snap_name), m_offset(off), + : m_image_ctx(image_ctx), + m_from_snap_id(from_snap_id), m_offset(off), m_length(len), m_include_parent(include_parent), m_whole_object(whole_object), m_callback(callback), m_callback_arg(callback_arg) diff --git a/src/librbd/api/Trash.cc b/src/librbd/api/Trash.cc index d8189e8a73d..c0924d9f5f6 100644 --- a/src/librbd/api/Trash.cc +++ b/src/librbd/api/Trash.cc @@ -483,8 +483,7 @@ int Trash::purge(IoCtx& io_ctx, time_t expire_ts, } r = librbd::api::DiffIterate::diff_iterate( - ictx, cls::rbd::UserSnapshotNamespace(), nullptr, 0, ictx->size, - false, true, + ictx, 0, 0, ictx->size, false, true, [](uint64_t offset, size_t len, int exists, void *arg) { auto *to_free = reinterpret_cast(arg); if (exists) diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc index 402c641c368..574e4722c54 100644 --- a/src/librbd/librbd.cc +++ b/src/librbd/librbd.cc @@ -2572,9 +2572,13 @@ namespace librbd { tracepoint(librbd, diff_iterate_enter, ictx, ictx->name.c_str(), ictx->snap_name.c_str(), ictx->read_only, fromsnapname, ofs, len, true, false); - int r = librbd::api::DiffIterate<>::diff_iterate(ictx, - cls::rbd::UserSnapshotNamespace(), - fromsnapname, ofs, + uint64_t from_snap_id = 0; + if (fromsnapname != nullptr) { + std::shared_lock image_locker{ictx->image_lock}; + from_snap_id = ictx->get_snap_id(cls::rbd::UserSnapshotNamespace(), + fromsnapname); + } + int r = librbd::api::DiffIterate<>::diff_iterate(ictx, from_snap_id, ofs, len, true, false, cb, arg); tracepoint(librbd, diff_iterate_exit, r); return r; @@ -2588,15 +2592,36 @@ namespace librbd { tracepoint(librbd, diff_iterate_enter, ictx, ictx->name.c_str(), ictx->snap_name.c_str(), ictx->read_only, fromsnapname, ofs, len, include_parent, whole_object); - int r = librbd::api::DiffIterate<>::diff_iterate(ictx, - cls::rbd::UserSnapshotNamespace(), - fromsnapname, ofs, - len, include_parent, + uint64_t from_snap_id = 0; + if (fromsnapname != nullptr) { + std::shared_lock image_locker{ictx->image_lock}; + from_snap_id = ictx->get_snap_id(cls::rbd::UserSnapshotNamespace(), + fromsnapname); + } + int r = librbd::api::DiffIterate<>::diff_iterate(ictx, from_snap_id, + ofs, len, include_parent, whole_object, cb, arg); tracepoint(librbd, diff_iterate_exit, r); return r; } + int Image::diff_iterate3(uint64_t from_snap_id, + uint64_t ofs, uint64_t len, uint32_t flags, + int (*cb)(uint64_t, size_t, int, void *), void *arg) + { + if ((flags & ~(RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT | + RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT)) != 0) { + return -EINVAL; + } + + ImageCtx *ictx = (ImageCtx *)ctx; + bool include_parent = flags & RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT; + bool whole_object = flags & RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT; + return librbd::api::DiffIterate<>::diff_iterate(ictx, from_snap_id, + ofs, len, include_parent, + whole_object, cb, arg); + } + ssize_t Image::write(uint64_t ofs, size_t len, bufferlist& bl) { ImageCtx *ictx = (ImageCtx *)ctx; @@ -6017,10 +6042,14 @@ extern "C" int rbd_diff_iterate(rbd_image_t image, tracepoint(librbd, diff_iterate_enter, ictx, ictx->name.c_str(), ictx->snap_name.c_str(), ictx->read_only, fromsnapname, ofs, len, true, false); - int r = librbd::api::DiffIterate<>::diff_iterate(ictx, - cls::rbd::UserSnapshotNamespace(), - fromsnapname, ofs, len, - true, false, cb, arg); + uint64_t from_snap_id = 0; + if (fromsnapname != nullptr) { + std::shared_lock image_locker{ictx->image_lock}; + from_snap_id = ictx->get_snap_id(cls::rbd::UserSnapshotNamespace(), + fromsnapname); + } + int r = librbd::api::DiffIterate<>::diff_iterate(ictx, from_snap_id, ofs, + len, true, false, cb, arg); tracepoint(librbd, diff_iterate_exit, r); return r; } @@ -6035,15 +6064,37 @@ extern "C" int rbd_diff_iterate2(rbd_image_t image, const char *fromsnapname, tracepoint(librbd, diff_iterate_enter, ictx, ictx->name.c_str(), ictx->snap_name.c_str(), ictx->read_only, fromsnapname, ofs, len, include_parent != 0, whole_object != 0); - int r = librbd::api::DiffIterate<>::diff_iterate(ictx, - cls::rbd::UserSnapshotNamespace(), - fromsnapname, ofs, len, - include_parent, whole_object, - cb, arg); + uint64_t from_snap_id = 0; + if (fromsnapname != nullptr) { + std::shared_lock image_locker{ictx->image_lock}; + from_snap_id = ictx->get_snap_id(cls::rbd::UserSnapshotNamespace(), + fromsnapname); + } + int r = librbd::api::DiffIterate<>::diff_iterate(ictx, from_snap_id, + ofs, len, include_parent, + whole_object, cb, arg); tracepoint(librbd, diff_iterate_exit, r); return r; } +extern "C" int rbd_diff_iterate3(rbd_image_t image, uint64_t from_snap_id, + uint64_t ofs, uint64_t len, uint32_t flags, + int (*cb)(uint64_t, size_t, int, void *), + void *arg) +{ + if ((flags & ~(RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT | + RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT)) != 0) { + return -EINVAL; + } + + librbd::ImageCtx *ictx = (librbd::ImageCtx *)image; + bool include_parent = flags & RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT; + bool whole_object = flags & RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT; + return librbd::api::DiffIterate<>::diff_iterate(ictx, from_snap_id, + ofs, len, include_parent, + whole_object, cb, arg); +} + extern "C" ssize_t rbd_write(rbd_image_t image, uint64_t ofs, size_t len, const char *buf) { diff --git a/src/pybind/rbd/c_rbd.pxd b/src/pybind/rbd/c_rbd.pxd index 8b604764a20..9624cb4ce05 100644 --- a/src/pybind/rbd/c_rbd.pxd +++ b/src/pybind/rbd/c_rbd.pxd @@ -59,6 +59,9 @@ cdef extern from "rbd/librbd.h" nogil: _RBD_WRITE_ZEROES_FLAG_THICK_PROVISION "RBD_WRITE_ZEROES_FLAG_THICK_PROVISION" + _RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT "RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT" + _RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT "RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT" + ctypedef void* rados_t ctypedef void* rados_ioctx_t ctypedef void* rbd_image_t @@ -597,6 +600,11 @@ cdef extern from "rbd/librbd.h" nogil: int (*cb)(uint64_t, size_t, int, void *) nogil except? -9000, void *arg) except? -9000 + int rbd_diff_iterate3(rbd_image_t image, uint64_t from_snap_id, + uint64_t ofs, uint64_t len, uint32_t flags, + int (*cb)(uint64_t, size_t, int, void *) + nogil except? -9000, + void *arg) except? -9000 int rbd_flush(rbd_image_t image) int rbd_invalidate_cache(rbd_image_t image) diff --git a/src/pybind/rbd/mock_rbd.pxi b/src/pybind/rbd/mock_rbd.pxi index 3c3c358fd04..04a056fcc64 100644 --- a/src/pybind/rbd/mock_rbd.pxi +++ b/src/pybind/rbd/mock_rbd.pxi @@ -59,6 +59,9 @@ cdef nogil: _RBD_WRITE_ZEROES_FLAG_THICK_PROVISION "RBD_WRITE_ZEROES_FLAG_THICK_PROVISION" + _RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT "RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT" + _RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT "RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT" + ctypedef void* rados_t ctypedef void* rados_ioctx_t ctypedef void* rbd_image_t @@ -740,6 +743,12 @@ cdef nogil: nogil except? -9000, void *arg) except? -9000: pass + int rbd_diff_iterate3(rbd_image_t image, uint64_t from_snap_id, + uint64_t ofs, uint64_t len, uint32_t flags, + int (*cb)(uint64_t, size_t, int, void *) + nogil except? -9000, + void *arg) except? -9000: + pass int rbd_flush(rbd_image_t image): pass diff --git a/src/pybind/rbd/rbd.pyx b/src/pybind/rbd/rbd.pyx index 334f4b63d65..10464821b63 100644 --- a/src/pybind/rbd/rbd.pyx +++ b/src/pybind/rbd/rbd.pyx @@ -3926,15 +3926,16 @@ cdef class Image(object): Raises :class:`InvalidArgument` if from_snapshot is after the currently set snapshot. - Raises :class:`ImageNotFound` if from_snapshot is not the name + Raises :class:`ImageNotFound` if from_snapshot is not the name or id of a snapshot of the image. :param offset: start offset in bytes :type offset: int :param length: size of region to report on, in bytes :type length: int - :param from_snapshot: starting snapshot name, or None - :type from_snapshot: str or None + :param from_snapshot: starting snapshot name or id, or None to + get all allocated extents + :type from_snapshot: str or int :param iterate_cb: function to call for each extent :type iterate_cb: function acception arguments for offset, length, and exists @@ -3945,18 +3946,45 @@ cdef class Image(object): :raises: :class:`InvalidArgument`, :class:`IOError`, :class:`ImageNotFound` """ - from_snapshot = cstr(from_snapshot, 'from_snapshot', opt=True) cdef: - char *_from_snapshot = opt_str(from_snapshot) + char *_from_snap_name = NULL + uint64_t _from_snap_id = 0 uint64_t _offset = offset, _length = length uint8_t _include_parent = include_parent uint8_t _whole_object = whole_object - with nogil: - ret = rbd_diff_iterate2(self.image, _from_snapshot, _offset, - _length, _include_parent, _whole_object, - &diff_iterate_cb, iterate_cb) + uint32_t _flags = 0 + + if from_snapshot is not None: + if isinstance(from_snapshot, str): + from_snap_name = cstr(from_snapshot, 'from_snapshot') + _from_snap_name = from_snap_name + elif isinstance(from_snapshot, int): + from_snap_name = None + _from_snap_id = from_snapshot + else: + raise TypeError("from_snapshot must be a string or an integer") + else: + from_snap_name = None + + if from_snap_name is not None: + with nogil: + ret = rbd_diff_iterate2(self.image, _from_snap_name, _offset, + _length, _include_parent, _whole_object, + &diff_iterate_cb, iterate_cb) + else: + if include_parent: + _flags |= _RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT + if whole_object: + _flags |= _RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT + with nogil: + ret = rbd_diff_iterate3(self.image, _from_snap_id, _offset, + _length, _flags, &diff_iterate_cb, + iterate_cb) if ret < 0: - msg = 'error generating diff from snapshot %s' % from_snapshot + if from_snap_name is not None: + msg = 'error generating diff from snapshot %s' % from_snapshot + else: + msg = 'error generating diff from snapshot id %d' % _from_snap_id raise make_ex(ret, msg) @requires_not_closed diff --git a/src/test/librbd/test_internal.cc b/src/test/librbd/test_internal.cc index d696e7c5087..2cc2f8b7c03 100644 --- a/src/test/librbd/test_internal.cc +++ b/src/test/librbd/test_internal.cc @@ -1298,8 +1298,7 @@ TEST_F(TestInternal, DiffIterateCloneOverwrite) { cls::rbd::UserSnapshotNamespace(), "one")); ASSERT_EQ(0, librbd::api::DiffIterate<>::diff_iterate( - ictx, cls::rbd::UserSnapshotNamespace(), nullptr, 0, size, true, false, - iterate_cb, (void *)&diff)); + ictx, 0, 0, size, true, false, iterate_cb, (void *)&diff)); ASSERT_EQ(one, diff); } diff --git a/src/test/pybind/test_rbd.py b/src/test/pybind/test_rbd.py index 77c1088f038..665f8569bbd 100644 --- a/src/test/pybind/test_rbd.py +++ b/src/test/pybind/test_rbd.py @@ -15,6 +15,7 @@ from assertions import (assert_equal as eq, assert_raises, assert_not_equal, assert_greater_equal) from datetime import datetime, timedelta from rados import (Rados, + LIBRADOS_SNAP_HEAD, LIBRADOS_OP_FLAG_FADVISE_DONTNEED, LIBRADOS_OP_FLAG_FADVISE_NOCACHE, LIBRADOS_OP_FLAG_FADVISE_RANDOM) @@ -683,14 +684,14 @@ class TestImage(object): self.image.write(data, 0) self.image.write_zeroes(0, 256) eq(self.image.read(256, 256), b'\0' * 256) - check_diff(self.image, 0, IMG_SIZE, None, []) + check_diff(self.image, 0, IMG_SIZE, None, 0, []) def test_write_zeroes_thick_provision(self): data = rand_data(256) self.image.write(data, 0) self.image.write_zeroes(0, 256, RBD_WRITE_ZEROES_FLAG_THICK_PROVISION) eq(self.image.read(256, 256), b'\0' * 256) - check_diff(self.image, 0, IMG_SIZE, None, [(0, 256, True)]) + check_diff(self.image, 0, IMG_SIZE, None, 0, [(0, 256, True)]) def test_read(self): data = self.image.read(0, 20) @@ -1380,22 +1381,146 @@ class TestImage(object): eq([], self.image.list_lockers()) def test_diff_iterate(self): - check_diff(self.image, 0, IMG_SIZE, None, []) + def cb(offset, length, exists): + raise Exception() + + assert_raises(TypeError, self.image.diff_iterate, 0, IMG_SIZE, 1.0, cb) + assert_raises(ImageNotFound, self.image.diff_iterate, 0, IMG_SIZE, + LIBRADOS_SNAP_HEAD, cb) + + check_diff(self.image, 0, IMG_SIZE, None, 0, []) self.image.write(b'a' * 256, 0) - check_diff(self.image, 0, IMG_SIZE, None, [(0, 256, True)]) + check_diff(self.image, 0, IMG_SIZE, None, 0, [(0, 256, True)]) self.image.write(b'b' * 256, 256) - check_diff(self.image, 0, IMG_SIZE, None, [(0, 512, True)]) + check_diff(self.image, 0, IMG_SIZE, None, 0, [(0, 512, True)]) self.image.discard(128, 256) - check_diff(self.image, 0, IMG_SIZE, None, [(0, 512, True)]) + check_diff(self.image, 0, IMG_SIZE, None, 0, [(0, 512, True)]) + + check_diff(self.image, 0, 0, None, 0, []) + check_diff(self.image, IMG_SIZE - 1, 0, None, 0, []) + check_diff(self.image, IMG_SIZE, 0, None, 0, []) self.image.create_snap('snap1') + snap_id1 = self.image.snap_get_id('snap1') + self.image.remove_snap('snap1') + assert_raises(ImageNotFound, self.image.diff_iterate, 0, IMG_SIZE, + 'snap1', cb) + assert_raises(ImageNotFound, self.image.diff_iterate, 0, IMG_SIZE, + snap_id1, cb) + + self.image.create_snap('snap1') + snap_id1 = self.image.snap_get_id('snap1') self.image.discard(0, 1 << IMG_ORDER) self.image.create_snap('snap2') + snap_id2 = self.image.snap_get_id('snap2') + + self.image.write(b'c' * 16, 16) + self.image.write(b'c', 1 << IMG_ORDER) + + self.image.set_snap('snap1') + check_diff(self.image, 0, IMG_SIZE, None, 0, [(0, 512, True)]) + check_diff(self.image, 0, IMG_SIZE, None, 0, + [(0, 1 << IMG_ORDER, True)], whole_object=True) + check_diff(self.image, 0, IMG_SIZE, 'snap1', snap_id1, []) + assert_raises(InvalidArgument, self.image.diff_iterate, 0, IMG_SIZE, + 'snap2', cb) + assert_raises(InvalidArgument, self.image.diff_iterate, 0, IMG_SIZE, + snap_id2, cb) + self.image.set_snap('snap2') - check_diff(self.image, 0, IMG_SIZE, 'snap1', [(0, 512, False)]) + check_diff(self.image, 0, IMG_SIZE, None, 0, []) + check_diff(self.image, 0, IMG_SIZE, 'snap1', snap_id1, + [(0, 512, False)]) + check_diff(self.image, 0, IMG_SIZE, 'snap1', snap_id1, + [(0, 1 << IMG_ORDER, False)], whole_object=True) + check_diff(self.image, 0, IMG_SIZE, 'snap2', snap_id2, []) + + self.image.set_snap(None) + expected_whole_object = [(0, 1 << IMG_ORDER, True), + (1 << IMG_ORDER, 1 << IMG_ORDER, True)] + check_diff(self.image, 0, IMG_SIZE, None, 0, + [(0, 32, True), (1 << IMG_ORDER, 1, True)]) + check_diff(self.image, 0, IMG_SIZE, None, 0, + expected_whole_object, whole_object=True) + check_diff(self.image, 0, IMG_SIZE, 'snap1', snap_id1, + [(0, 32, True), + (32, 480, False), + (1 << IMG_ORDER, 1, True)]) + check_diff(self.image, 0, IMG_SIZE, 'snap1', snap_id1, + expected_whole_object, whole_object=True) + check_diff(self.image, 0, IMG_SIZE, 'snap2', snap_id2, + [(0, 32, True), (1 << IMG_ORDER, 1, True)]) + check_diff(self.image, 0, IMG_SIZE, 'snap2', snap_id2, + expected_whole_object, whole_object=True) + self.image.remove_snap('snap1') self.image.remove_snap('snap2') + @require_features([RBD_FEATURE_LAYERING]) + def test_diff_iterate_from_trash_snap(self): + def cb(offset, length, exists): + raise Exception() + + self.image.write(b'a' * 256, 0) + self.image.create_snap('snap') + snap_id = self.image.snap_get_id('snap') + clone_name = get_temp_image_name() + self.rbd.clone(ioctx, image_name, 'snap', ioctx, clone_name, features, + clone_format=2) + + self.image.write(b'b' * 256, 256) + check_diff(self.image, 0, IMG_SIZE, None, 0, [(0, 512, True)]) + check_diff(self.image, 0, IMG_SIZE, 'snap', snap_id, [(256, 256, True)]) + + self.image.remove_snap('snap') + image_snaps = list(self.image.list_snaps()) + assert [s['namespace'] for s in image_snaps] == [RBD_SNAP_NAMESPACE_TYPE_TRASH] + assert image_snaps[0]['id'] == snap_id + + assert_raises(ImageNotFound, self.image.diff_iterate, 0, IMG_SIZE, + 'snap', cb) + check_diff_one(self.image, 0, IMG_SIZE, snap_id, [(256, 256, True)]) + self.image.write(b'c' * 256, 128) + check_diff(self.image, 0, IMG_SIZE, None, 0, [(0, 512, True)]) + check_diff_one(self.image, 0, IMG_SIZE, snap_id, [(128, 384, True)]) + check_diff_one(self.image, 0, IMG_SIZE, snap_id, + [(0, 1 << IMG_ORDER, True)], whole_object=True) + + self.rbd.remove(ioctx, clone_name) + assert_raises(ImageNotFound, self.image.diff_iterate, 0, IMG_SIZE, + 'snap', cb) + assert_raises(ImageNotFound, self.image.diff_iterate, 0, IMG_SIZE, + snap_id, cb) + + @require_features([RBD_FEATURE_LAYERING]) + def test_diff_iterate_exclude_parent(self): + self.image.write(b'a' * 256, 0) + self.image.create_snap('snap') + clone_name = get_temp_image_name() + self.rbd.clone(ioctx, image_name, 'snap', ioctx, clone_name, features, + clone_format=2) + + self.image.write(b'b' * 256, 256) + check_diff(self.image, 0, IMG_SIZE, None, 0, [(0, 512, True)]) + + with Image(ioctx, clone_name) as clone: + check_diff(clone, 0, IMG_SIZE, None, 0, [(0, 256, True)]) + clone.write(b'c' * 256, 1 << IMG_ORDER) + check_diff(clone, 0, IMG_SIZE, None, 0, + [(0, 256, True), (1 << IMG_ORDER, 256, True)]) + check_diff(clone, 0, IMG_SIZE, None, 0, + [(0, 1 << IMG_ORDER, True), + (1 << IMG_ORDER, 1 << IMG_ORDER, True)], + whole_object=True) + check_diff(clone, 0, IMG_SIZE, None, 0, + [(1 << IMG_ORDER, 256, True)], include_parent=False) + check_diff(clone, 0, IMG_SIZE, None, 0, + [(1 << IMG_ORDER, 1 << IMG_ORDER, True)], + include_parent=False, whole_object=True) + + self.rbd.remove(ioctx, clone_name) + self.image.remove_snap('snap') + def test_aio_read(self): # this is a list so that the local cb() can modify it retval = [None] @@ -1621,13 +1746,20 @@ class TestImageId(object): info = self.image2.stat() check_stat(info, new_size, IMG_ORDER) -def check_diff(image, offset, length, from_snapshot, expected): +def check_diff_one(image, offset, length, from_snapshot, expected, **kwargs): extents = [] def cb(offset, length, exists): extents.append((offset, length, exists)) - image.diff_iterate(0, IMG_SIZE, from_snapshot, cb) + image.diff_iterate(offset, length, from_snapshot, cb, **kwargs) eq(extents, expected) +def check_diff(image, offset, length, from_snap_name, from_snap_id, expected, + **kwargs): + assert from_snap_name is None or isinstance(from_snap_name, str) + assert isinstance(from_snap_id, int) + check_diff_one(image, offset, length, from_snap_name, expected, **kwargs) + check_diff_one(image, offset, length, from_snap_id, expected, **kwargs) + class TestClone(object): @require_features([RBD_FEATURE_LAYERING]) @@ -2871,6 +3003,37 @@ class TestGroups(object): self.group.remove_snap(new_snap_name) eq([], list(self.group.list_snaps())) + def test_group_snap_diff_iterate(self): + def cb(offset, length, exists): + raise Exception() + + self.image.write(b'a' * 256, 0) + self.group.add_image(ioctx, image_name) + self.group.create_snap(snap_name) + image_snaps = list(self.image.list_snaps()) + assert [s['namespace'] for s in image_snaps] == [RBD_SNAP_NAMESPACE_TYPE_GROUP] + image_snap_name = image_snaps[0]['name'] + image_snap_id = image_snaps[0]['id'] + + self.image.write(b'b' * 256, 256) + check_diff(self.image, 0, IMG_SIZE, None, 0, [(0, 512, True)]) + assert_raises(ImageNotFound, self.image.diff_iterate, 0, IMG_SIZE, + image_snap_name, cb) + check_diff_one(self.image, 0, IMG_SIZE, image_snap_id, + [(256, 256, True)]) + self.image.write(b'c' * 256, 128) + check_diff(self.image, 0, IMG_SIZE, None, 0, [(0, 512, True)]) + check_diff_one(self.image, 0, IMG_SIZE, image_snap_id, + [(128, 384, True)]) + check_diff_one(self.image, 0, IMG_SIZE, image_snap_id, + [(0, 1 << IMG_ORDER, True)], whole_object=True) + + self.group.remove_snap(snap_name) + assert_raises(ImageNotFound, self.image.diff_iterate, 0, IMG_SIZE, + image_snap_name, cb) + assert_raises(ImageNotFound, self.image.diff_iterate, 0, IMG_SIZE, + image_snap_id, cb) + def test_group_snap_rollback(self): for _ in range(1, 3): create_image() From 8d014f171fe00a1aaa371c7fac9a2ac31640f0ab Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Mon, 3 Mar 2025 17:59:35 +0100 Subject: [PATCH 2/2] test/pybind/rbd: fix read offset in write zeroes tests Random data is written and write zeroes is invoked on 0~256, but the read is done on 256~256. This means that if write zeroes malfunctions the test wouldn't catch it (especially in the thick provision case). Signed-off-by: Ilya Dryomov (cherry picked from commit d41f0fa01f59a8d056dc28934b92212c78a05a62) --- src/test/pybind/test_rbd.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/pybind/test_rbd.py b/src/test/pybind/test_rbd.py index 665f8569bbd..6339fd3f248 100644 --- a/src/test/pybind/test_rbd.py +++ b/src/test/pybind/test_rbd.py @@ -683,14 +683,14 @@ class TestImage(object): data = rand_data(256) self.image.write(data, 0) self.image.write_zeroes(0, 256) - eq(self.image.read(256, 256), b'\0' * 256) + eq(self.image.read(0, 256), b'\0' * 256) check_diff(self.image, 0, IMG_SIZE, None, 0, []) def test_write_zeroes_thick_provision(self): data = rand_data(256) self.image.write(data, 0) self.image.write_zeroes(0, 256, RBD_WRITE_ZEROES_FLAG_THICK_PROVISION) - eq(self.image.read(256, 256), b'\0' * 256) + eq(self.image.read(0, 256), b'\0' * 256) check_diff(self.image, 0, IMG_SIZE, None, 0, [(0, 256, True)]) def test_read(self): @@ -1591,7 +1591,7 @@ class TestImage(object): eq(retval[0], 0) eq(comp.get_return_value(), 0) eq(sys.getrefcount(comp), 2) - eq(self.image.read(256, 256), b'\0' * 256) + eq(self.image.read(0, 256), b'\0' * 256) def test_aio_flush(self): retval = [None]