From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47198) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adHYC-0002Bd-Hw for qemu-devel@nongnu.org; Tue, 08 Mar 2016 08:19:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adHYB-0004M5-Ez for qemu-devel@nongnu.org; Tue, 08 Mar 2016 08:19:48 -0500 Date: Tue, 8 Mar 2016 14:19:16 +0100 From: Niels de Vos Message-ID: <20160308131916.GF18666@ndevos-x240.usersys.redhat.com> References: <1457373855-8072-1-git-send-email-ndevos@redhat.com> <20160307182738.GA14127@localhost.localdomain> <20160308042148.GB8006@ndevos-x240.usersys.redhat.com> <20160308125326.GE5807@noname.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160308125326.GE5807@noname.str.redhat.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH] block/gluster: add support for SEEK_DATA/SEEK_HOLE List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Jeff Cody , qemu-block@nongnu.org, Prasanna Kumar Kalever , qemu-devel@nongnu.org On Tue, Mar 08, 2016 at 01:53:26PM +0100, Kevin Wolf wrote: > Am 08.03.2016 um 05:21 hat Niels de Vos geschrieben: > > On Mon, Mar 07, 2016 at 01:27:38PM -0500, Jeff Cody wrote: > > > On Mon, Mar 07, 2016 at 07:04:15PM +0100, Niels de Vos wrote: > > > > GlusterFS 3.8 contains support for SEEK_DATA and SEEK_HOLE. This makes > > > > it possible to detect sparse areas in files. > > > > > > > > Signed-off-by: Niels de Vos > > > > > > > > -- > > > > Tested by compiling and running "qemu-img map gluster://..." with a > > > > build of the current master branch of glusterfs. Using a Fedora > > > > cloud image (in raw format) shows many SEEK procudure calls going back > > > > and forth over the network. The output of "qemu map" matches the output > > > > when run against the image on the local filesystem. > > > > --- > > > > block/gluster.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > configure | 25 +++++++++ > > > > 2 files changed, 184 insertions(+) > > > > > > > > diff --git a/block/gluster.c b/block/gluster.c > > > > index 65077a0..1430010 100644 > > > > --- a/block/gluster.c > > > > +++ b/block/gluster.c > > > > @@ -677,6 +677,153 @@ static int qemu_gluster_has_zero_init(BlockDriverState *bs) > > > > return 0; > > > > } > > > > > > > > +#ifdef CONFIG_GLUSTERFS_SEEK_DATA > > > > > > Why do we need to make this a compile-time option? Version checking > > > is problematic; for instance, different distributions may have > > > backported bug fixes / features, that are not reflected by the > > > reported version number, etc.. Ideally, we can determine > > > functionality during runtime, and behave accordingly. > > > > This will not get backported to older Gluster versions, it required a > > protocol change. > > > > > If SEEK_DATA and SEEK_HOLE are not supported, > > > qemu_gluster_co_get_block_status can return that sectors are all > > > allocated (which is what happens in block/io.c anyway if the driver > > > doesn't support the function). > > > > Ok, good to know. > > > > > As long as glfs_lseek() will return error (e.g. EINVAL) for an invalid > > > whence value, we can handle it runtime. Does glfs_lseek() behave > > > sanely? > > > > Unfortunately older versions of libgfapi do not return EINVAL when > > SEEK_DATA/HOLE is used. It is something we'll need to fix in the stable > > releases. We can not assume that all users have installed a version of > > the library that handles SEEK_DATA/HOLE correctly (return EINVAL) when > > there is no support in the network protocol or on the server. > > > > To be sure that we don't get some undefined behaviour, the compile time > > check is needed. > > The code could be compiled on a host with newer libgfapi, but run on a > different host with an older version. This is why having (only) compile > time checks is rarely a good idea. Oh, yes, that is possible. glfs_lseek() is not a new function, so the symbol version did not need to change. > Jeff's suggestion to probe the actual behaviour on the host we're > running on in .bdrv_open() sounds reasonable to me. Yes, it sure is. I'll send a v2 patch soon. Thanks, Niels