Re: [Libguestfs] [nbdkit PATCH 2/4] truncate: Factor out reading real_size under mutex

On Wed, Apr 24, 2019 at 05:24:39PM -0500, Eric Blake wrote: > Add a helper function get_real_size() to make it easier to add sanity > checking that mutex calls don't fail. This patch is fine, ACK. > I'm a bit unsure why truncate.c needs a lock anyway. I guess it's > because we are storing 'size' and 'real_size' as globals rather than > per-connection values in the handle, and we're worrying about parallel > connections where we don't want one thread reading inconsistent values > while another thread is in the middle of truncate_get_size()? At any > rate, as long as we don't have dynamic resize in the NBD protocol, > size can't change within the confines of a single connection. And even > if we DO assume that the underlying plugin reports different sizes for > different connections, our use of a global does not play well (if > client A sees size 1k, then client B sees size 2k, that changes client > A's use of 'real_size' to be something different than client A was > expecting). > > So, I think we have to move 'size' and 'real_size' into the > per-connection handle, at which point, can't we just set them once > during truncate_prepare by moving the existing guts of > truncate_get_size there, and then letting truncate_get_size just > return h->size while all other truncate_* functions just access > h->real_size without a lock? Even storing fields in the handle is not necessarily safe when using the parallel thread model, because you can have multiple requests outstanding on the same handle. (However I believe it is safe given the current implementation of get_size / lack of resize support in nbdkit.)