| Message ID | 20200131174436.2961874-8-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve qcow2 all-zero detection | expand |
31.01.2020 20:44, Eric Blake wrote: > block.c already defaults to 0 if we don't provide a callback; there's > no need to write a callback that always fails. > > Signed-off-by: Eric Blake<eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On Fri 31 Jan 2020 06:44:26 PM CET, Eric Blake wrote: > block.c already defaults to 0 if we don't provide a callback; there's > no need to write a callback that always fails. > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
On Fri, Jan 31, 2020 at 11:44:26AM -0600, Eric Blake wrote: > block.c already defaults to 0 if we don't provide a callback; there's > no need to write a callback that always fails. > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Niels de Vos <ndevos@redhat.com> > --- > block/gluster.c | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/block/gluster.c b/block/gluster.c > index 4fa4a77a4777..9d952c70981b 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -1357,12 +1357,6 @@ static int64_t qemu_gluster_allocated_file_size(BlockDriverState *bs) > } > } > > -static int qemu_gluster_has_zero_init(BlockDriverState *bs) > -{ > - /* GlusterFS volume could be backed by a block device */ > - return 0; > -} > - > /* > * Find allocation range in @bs around offset @start. > * May change underlying file descriptor's file offset. > @@ -1567,8 +1561,6 @@ static BlockDriver bdrv_gluster = { > .bdrv_co_readv = qemu_gluster_co_readv, > .bdrv_co_writev = qemu_gluster_co_writev, > .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, > - .bdrv_has_zero_init = qemu_gluster_has_zero_init, > - .bdrv_has_zero_init_truncate = qemu_gluster_has_zero_init, > #ifdef CONFIG_GLUSTERFS_DISCARD > .bdrv_co_pdiscard = qemu_gluster_co_pdiscard, > #endif > @@ -1599,8 +1591,6 @@ static BlockDriver bdrv_gluster_tcp = { > .bdrv_co_readv = qemu_gluster_co_readv, > .bdrv_co_writev = qemu_gluster_co_writev, > .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, > - .bdrv_has_zero_init = qemu_gluster_has_zero_init, > - .bdrv_has_zero_init_truncate = qemu_gluster_has_zero_init, > #ifdef CONFIG_GLUSTERFS_DISCARD > .bdrv_co_pdiscard = qemu_gluster_co_pdiscard, > #endif > @@ -1631,8 +1621,6 @@ static BlockDriver bdrv_gluster_unix = { > .bdrv_co_readv = qemu_gluster_co_readv, > .bdrv_co_writev = qemu_gluster_co_writev, > .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, > - .bdrv_has_zero_init = qemu_gluster_has_zero_init, > - .bdrv_has_zero_init_truncate = qemu_gluster_has_zero_init, > #ifdef CONFIG_GLUSTERFS_DISCARD > .bdrv_co_pdiscard = qemu_gluster_co_pdiscard, > #endif > @@ -1669,8 +1657,6 @@ static BlockDriver bdrv_gluster_rdma = { > .bdrv_co_readv = qemu_gluster_co_readv, > .bdrv_co_writev = qemu_gluster_co_writev, > .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, > - .bdrv_has_zero_init = qemu_gluster_has_zero_init, > - .bdrv_has_zero_init_truncate = qemu_gluster_has_zero_init, > #ifdef CONFIG_GLUSTERFS_DISCARD > .bdrv_co_pdiscard = qemu_gluster_co_pdiscard, > #endif > -- > 2.24.1 > > _______________________________________________ > integration mailing list > integration@gluster.org > https://lists.gluster.org/mailman/listinfo/integration >
On 2/17/20 2:06 AM, Niels de Vos wrote: > On Fri, Jan 31, 2020 at 11:44:26AM -0600, Eric Blake wrote: >> block.c already defaults to 0 if we don't provide a callback; there's >> no need to write a callback that always fails. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > Reviewed-by: Niels de Vos <ndevos@redhat.com> > Per your other message, On 2/17/20 2:16 AM, Niels de Vos wrote: > On Fri, Jan 31, 2020 at 11:44:31AM -0600, Eric Blake wrote: >> Since gluster already copies file-posix for lseek usage in block >> status, it also makes sense to copy it for learning if the image >> currently reads as all zeroes. >> >> +static int qemu_gluster_known_zeroes(BlockDriverState *bs) >> +{ >> + /* >> + * GlusterFS volume could be backed by a block device, with no way > > Actually, Gluster dropped support for volumes backed by block devices > (LVM) a few releases back. Nobody could be found that used it, and it > could not be combined with other Gluster features. All contents on a > Gluster volume is now always backed by 'normal' files on a filesystem. That's useful to know. Thanks! > > Creation or truncation should behave just as on a file on a local > filesystem. So maybe qemu_gluster_known_zeroes is not needed at all? Which version of gluster first required a regular filesystem backing for all gluster files? Does qemu support older versions (in which case, what is the correct version-probing invocation to return 0 prior to that point, and 1 after), or do all versions supported by qemu already guarantee zero initialization on creation or widening truncation by virtue of POSIX file semantics (in which case, patch 7 should instead switch to using .bdrv_has_zero_init_1 for both functions)? Per configure, we probe for glusterfs_xlator_opt from gluster 4, which implies the code still tries to be portable to even older gluster, but I'm not sure if this squares with qemu-doc.texi which mentions our minimum distro policy (for example, now that qemu requires python 3 consistent with our distro policy, that rules out several older systems where older gluster was likely to be present). I'm respinning the series for other reasons, but would like to get this right as part of that respin.
On 2/17/20 6:03 AM, Eric Blake wrote: > > > > Creation or truncation should behave just as on a file on a local > > filesystem. So maybe qemu_gluster_known_zeroes is not needed at all? > > Which version of gluster first required a regular filesystem backing for > all gluster files? Does qemu support older versions (in which case, > what is the correct version-probing invocation to return 0 prior to that > point, and 1 after), or do all versions supported by qemu already > guarantee zero initialization on creation or widening truncation by > virtue of POSIX file semantics (in which case, patch 7 should instead > switch to using .bdrv_has_zero_init_1 for both functions)? Per > configure, we probe for glusterfs_xlator_opt from gluster 4, which > implies the code still tries to be portable to even older gluster, but > I'm not sure if this squares with qemu-doc.texi which mentions our > minimum distro policy (for example, now that qemu requires python 3 > consistent with our distro policy, that rules out several older systems > where older gluster was likely to be present). For reference, I quickly found commit efc6c070ac as an example of bumping minimum versions (however, that commit is from 2018, so I'm sure there are even more recent examples, just not with the same keywords that I was searching for).
On Mon, Feb 17, 2020 at 06:03:40AM -0600, Eric Blake wrote: > On 2/17/20 2:06 AM, Niels de Vos wrote: > > On Fri, Jan 31, 2020 at 11:44:26AM -0600, Eric Blake wrote: > > > block.c already defaults to 0 if we don't provide a callback; there's > > > no need to write a callback that always fails. > > > > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > > > Reviewed-by: Niels de Vos <ndevos@redhat.com> > > > > Per your other message, > > On 2/17/20 2:16 AM, Niels de Vos wrote: > > On Fri, Jan 31, 2020 at 11:44:31AM -0600, Eric Blake wrote: > >> Since gluster already copies file-posix for lseek usage in block > >> status, it also makes sense to copy it for learning if the image > >> currently reads as all zeroes. > >> > > >> +static int qemu_gluster_known_zeroes(BlockDriverState *bs) > >> +{ > >> + /* > >> + * GlusterFS volume could be backed by a block device, with no way > > > > Actually, Gluster dropped support for volumes backed by block devices > > (LVM) a few releases back. Nobody could be found that used it, and it > > could not be combined with other Gluster features. All contents on a > > Gluster volume is now always backed by 'normal' files on a filesystem. > > That's useful to know. Thanks! > > > > > Creation or truncation should behave just as on a file on a local > > filesystem. So maybe qemu_gluster_known_zeroes is not needed at all? > > Which version of gluster first required a regular filesystem backing for all > gluster files? Does qemu support older versions (in which case, what is the > correct version-probing invocation to return 0 prior to that point, and 1 > after), or do all versions supported by qemu already guarantee zero > initialization on creation or widening truncation by virtue of POSIX file > semantics (in which case, patch 7 should instead switch to using > .bdrv_has_zero_init_1 for both functions)? Per configure, we probe for > glusterfs_xlator_opt from gluster 4, which implies the code still tries to > be portable to even older gluster, but I'm not sure if this squares with > qemu-doc.texi which mentions our minimum distro policy (for example, now > that qemu requires python 3 consistent with our distro policy, that rules > out several older systems where older gluster was likely to be present). The block device feature (storage/bd xlator) got deprecated in Gluster 5.0, and was removed with Gluster 6.0. Fedora 29 is the last version that contained the bd.so xlator (glusterfs-server 5.0, deprecated). All currently maintained and available Gluster releases should have glusterfs_xlator_opt (introduced with glusterfs-3.5 in 2014). However, I am not sure what versions are provided with different distributions. The expectation is that at least Gluster 5 is provided, as older releases will not get any updates anymore. See https://www.gluster.org/release-schedule/ for a more detailed timeline. Unfortunately there is no reasonable way to probe for the type of backend (block or filesystem) that is used. So, a runtime check to be on the extreme safe side to fallback on block device backends is not an option. HTH, Niels
diff --git a/block/gluster.c b/block/gluster.c index 4fa4a77a4777..9d952c70981b 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -1357,12 +1357,6 @@ static int64_t qemu_gluster_allocated_file_size(BlockDriverState *bs) } } -static int qemu_gluster_has_zero_init(BlockDriverState *bs) -{ - /* GlusterFS volume could be backed by a block device */ - return 0; -} - /* * Find allocation range in @bs around offset @start. * May change underlying file descriptor's file offset. @@ -1567,8 +1561,6 @@ static BlockDriver bdrv_gluster = { .bdrv_co_readv = qemu_gluster_co_readv, .bdrv_co_writev = qemu_gluster_co_writev, .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, - .bdrv_has_zero_init = qemu_gluster_has_zero_init, - .bdrv_has_zero_init_truncate = qemu_gluster_has_zero_init, #ifdef CONFIG_GLUSTERFS_DISCARD .bdrv_co_pdiscard = qemu_gluster_co_pdiscard, #endif @@ -1599,8 +1591,6 @@ static BlockDriver bdrv_gluster_tcp = { .bdrv_co_readv = qemu_gluster_co_readv, .bdrv_co_writev = qemu_gluster_co_writev, .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, - .bdrv_has_zero_init = qemu_gluster_has_zero_init, - .bdrv_has_zero_init_truncate = qemu_gluster_has_zero_init, #ifdef CONFIG_GLUSTERFS_DISCARD .bdrv_co_pdiscard = qemu_gluster_co_pdiscard, #endif @@ -1631,8 +1621,6 @@ static BlockDriver bdrv_gluster_unix = { .bdrv_co_readv = qemu_gluster_co_readv, .bdrv_co_writev = qemu_gluster_co_writev, .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, - .bdrv_has_zero_init = qemu_gluster_has_zero_init, - .bdrv_has_zero_init_truncate = qemu_gluster_has_zero_init, #ifdef CONFIG_GLUSTERFS_DISCARD .bdrv_co_pdiscard = qemu_gluster_co_pdiscard, #endif @@ -1669,8 +1657,6 @@ static BlockDriver bdrv_gluster_rdma = { .bdrv_co_readv = qemu_gluster_co_readv, .bdrv_co_writev = qemu_gluster_co_writev, .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, - .bdrv_has_zero_init = qemu_gluster_has_zero_init, - .bdrv_has_zero_init_truncate = qemu_gluster_has_zero_init, #ifdef CONFIG_GLUSTERFS_DISCARD .bdrv_co_pdiscard = qemu_gluster_co_pdiscard, #endif
block.c already defaults to 0 if we don't provide a callback; there's no need to write a callback that always fails. Signed-off-by: Eric Blake <eblake@redhat.com> --- block/gluster.c | 14 -------------- 1 file changed, 14 deletions(-)