From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46512) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y4aM5-00005h-08 for qemu-devel@nongnu.org; Fri, 26 Dec 2014 14:15:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y4aM0-0002Ug-UX for qemu-devel@nongnu.org; Fri, 26 Dec 2014 14:15:20 -0500 Received: from relay.parallels.com ([195.214.232.42]:59909) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y4aM0-0002UM-MT for qemu-devel@nongnu.org; Fri, 26 Dec 2014 14:15:16 -0500 Message-ID: <549DB3B6.1070800@openvz.org> Date: Fri, 26 Dec 2014 22:15:02 +0300 From: "Denis V. Lunev" MIME-Version: 1.0 References: <1419597313-20514-1-git-send-email-den@openvz.org> <1419597313-20514-2-git-send-email-den@openvz.org> <549D5F16.2070007@kamp.de> <549D6351.8050904@parallels.com> In-Reply-To: <549D6351.8050904@parallels.com> Content-Type: text/plain; charset="ISO-8859-15"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/7] block: fix maximum length sent to bdrv_co_do_write_zeroes callback in bs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 26/12/14 16:32, Denis V. Lunev wrote: > On 26/12/14 16:13, Peter Lieven wrote: >> Am 26.12.2014 um 13:35 schrieb Denis V. Lunev: >>> The check for maximum length was added by >>> commit c31cb70728d2c0c8900b35a66784baa446fd5147 >>> Author: Peter Lieven >>> Date: Thu Oct 24 12:06:58 2013 +0200 >>> block: honour BlockLimits in bdrv_co_do_write_zeroes >>> >>> but actually if driver provides .bdrv_co_write_zeroes callback, >>> there is >>> no need to limit the size to 32 MB. Callback should provide effective >>> implementation which normally should not write any zeroes in comparable >>> amount. >> >> NACK. >> >> First there is no guarantee that bdrv_co_do_write_zeroes is a fast >> operation. >> This heaviliy depends on several circumstances that the block layer >> is not aware of. >> If a specific protocol knows it is very fast in writing zeroes under >> any circumstance >> it should provide INT_MAX in bs->bl.max_write_zeroes. It is then >> still allowed to >> return -ENOTSUP if the request size or alignment doesn't fit. > > the idea is that (from my point of view) if .bdrv_co_do_write_zeroes is > specified, the cost is almost the same for any amount of zeroes > written. This is true for fallocate from my point of view. The amount > of actually written data will be in several orders less than specified > except slow path, which honors 32 MB limit. > > If the operation is complex in realization, then it will be rate-limited > below, in actual implementation. > >> There are known backends e.g. anything that deals with SCSI that have >> a known >> limitation of the maximum number of zeroes they can write fast in a >> single request. >> This number MUST NOT be exceeded. The below patch would break all >> those backends. > > could you pls point me this backends. Actually, from my point of > view, they should properly setup max_write_zeroes for themselves. > This is done at least in block/iscsi.c and it would be consistent > way of doing so. > >> >> What issue are you trying to fix with this patch? Maybe there is a >> better way to fix >> it at another point in the code. >> > > I am trying to minimize amount of metadata updates for a file. > This provides some speedup even on ext4 and this will provide > even more speedup with a distributed filesystem like CEPH > where size updates of the files and block allocation are > costly. > > Regards, > Den First of all, the patch is really wrong :) It was written using wrong assumptions. OK. I have spent some time reading your original patchset and and did not found any useful justification for default limit for both discard and write zero. Yes, there are drivers which requires block level to call .bdrv_co_do_write_zeroes with alignment and with upper limit. But in this case driver setups max_write_zeroes. All buggy drivers should do that not to affect not buggy ones from my opinion. This is the only purpose of the original patches for limits. I have wrongly interpret BlockLimits as something connected with time of the operation. Sorry for that. Therefore there is no good reason for limiting the amount of data sent to drv->bdrv_co_writev with any data size. The only thing is that it would be good not to allocate too many memory at once. We could do something like base = qemu_try_blockalign(bs, MIN(2048, num) * BDRV_SECTOR_SIZE); added = 0; for (added = 0; added < num; add += MIN(2048, num)) { qemu_iovec_add(qiov, base, MIN(2048, num)); } to avoid really big allocations here even if .max_write_zeroes is very high. Do you think that this might be useful? As for .bdrv_co_do_write_zeroes itself, can we still drop default 32 Mb limit? If there are some buggy drivers, they should have .max_write_zeroes specified. The same applies to .max_discard Regards, Den