Re: [Qemu-stable] [Nbd] [Qemu-devel] [PATCH] nbd: fix trim/discard commands with a length bigger than NBD_MAX_BUFFER_SIZE

[Michał Belczyk]

On Tue, May 10, 2016 at 04:41:27PM +0100, Alex Bligh wrote:
> On 10 May 2016, at 16:29, Eric Blake <address@hidden> wrote:
>
> > So the kernel is currently one of the clients that does NOT honor block
> > sizes, and as such, servers should be prepared for ANY size up to
> > UINT_MAX (other than DoS handling).
>
> Interesting followup question:
>
> If the kernel does not fragment TRIM requests at all (in the
> same way it fragments read and write requests), I suspect
> something bad may happen with TRIM requests over 2^31
> in size (particularly over 2^32 in size), as the length
> field in nbd only has 32 bits.
>
> Whether it supports block size constraints or not, it is
> going to need to do *some* breaking up of requests.

Doesn't the kernel break TRIM requests at max_discard_sectors?

I remember testing the following change in the nbd kmod long time ago:

#if 0
                disk->queue->limits.max_discard_sectors = UINT_MAX;
#else
                disk->queue->limits.max_discard_sectors = 65536;
#endif

The problem with large TRIM requests is exactly the same as with other 
(READ/WRITE) large requests -- they _may_ take loads of time and if the client 
wants to support a fast switch over to another replica it must put some 
constraints on the timeout value... which may be very easily broken if you 
allow things like a 1GB trim request on the server using DIO on the other side 
(and say heavily fragmented sparse file over XFS, never mind).

I don't think it's the problem of QEMU NBD server to fix that, the constraint 
on the server side is perfectly fine, the problem is to note that a change on 
the client side is required to limit the maximum size of the TRIM requests.
The reason for the patch I pasted above was that at the time I looked into it 
there was no other way to change the TRIM request size via 
/sys/block/$dev/queue/max_discard_sectors, but maybe the more recent kernels 
allow that?
Not to mention that the NBD client option to set that at NBD queue setup time 
would be nice...

Just my 2p.

--
Michał Belczyk Snr