Re: [Nbd] [RFC] [PATCH] support FUA and FLUSH

[Wouter Verhelst <w@...112...>]

On Mon, May 16, 2011 at 04:28:56PM +0100, Alex Bligh wrote:
> Wouter,
> 
> --On 16 May 2011 15:28:32 +0200 Wouter Verhelst <w@...112...> wrote:
> 
> >>diff --git a/cliserv.h b/cliserv.h
> >>index 51c8bd1..63c2743 100644
> >>--- a/cliserv.h
> >>+++ b/cliserv.h
> ....
> >>@@ -135,6 +140,9 @@ u64 ntohll(u64 a) {
> >> /* Flags used between the client and server */
> >> # define NBD_FLAG_HAS_FLAGS	(1 << 0)	/* Flags are there */
> >> # define NBD_FLAG_READ_ONLY	(1 << 1)	/* Device is read-only */
> >>+#define NBD_FLAG_SEND_FLUSH	(1 << 2)	/* Send FLUSH */
> >>+#define NBD_FLAG_SEND_FUA	(1 << 3)	/* Send FLUSH */
> >
> >"Send FUA" rather than "Send FLUSH"? Also, please expand what 'FUA'
> >stands for in this comment.
> 
> Fixed
> 
> >>+#define NBD_FLAG_ROTATIONAL	(1 << 3)	/* Use elevator algorithm -
> >>rotational media */
> >
> >That's the same value as the _SEND_FUA thing here; should that not be
> >different?
> 
> Fixed
> 
> >>+/* For sync_file_range */
> >>+#define _GNU_SOURCE
> >>+
> >
> >This should perhaps be inside an #ifdef HAVE_SYNC_FILE_RANGE, too.
> 
> This is (surprisingly) non-trivial. HAVE_SYNC_FILE_RANGE isn't defined
> until config.h is included. config.h is supplied by lfs.h. However,
> whether NBD_LFS is set or not, if _GNU_SOURCE is set, types.h will
> (through features.h) set _LARGEFILE_SOURCE but (confusingly) not
> _FILE_OFFSET_BITS.

That's because they serve different purposes. Setting _LARGEFILE_SOURCE
means "I want to be able to open and use files that are larger than
2GB". It doesn't change any semantics, it just allows one to do more

Setting _FILE_OFFSET_BITS changes the definition of off_t to a 64-bit
integer on 32-bit systems (rather than a 32-bit one) and changes all
relevant syscalls to behave like their ...64() variants. This does
change semantics, so setting it indiscriminately might not be a very
good idea.

> Given sync_file_range() is 64 bit only, I think the best solution is
> for lfs.h to define USE_SYNC_FILE_RANGE only if NBD_LFS is set *and*
> HAVE_SYNC_FILE_RANGE is true. That can then set _GNU_SOURCE without
> risking changing the file offset bits halfway through the headers.
> 
> This is pretty disgusting but I can't see any other way.

Er, right. Whoa. Didn't realize the includes were getting this hairy.

Well, nvm then; use your own discretion. But the comment shouldn't be
removed, so that it's clear nbd-server should still work on non-GNU
systems.

> One thing that would make it cleaner is for cliserv.h to avoid
> manipulating _LARGEFILE_SOURCE too. I don't think it needs to,
> and it seems to me simply including "lfs.h" would do.

Ah, yes. That's an artifact of the time from before when lfs.h existed.
I didn't even know it was still in there.

> That's unless there's some distinction I've been missing between
> NBD_LFS merely defined, and set to 1 (the checks are different).

No, there shouldn't be.

> However, I am aware that fiddling with LFS code can lead to subtle
> errors.

Yeah. OTOH, I don't think I still support non-LFS operation these days.
Initially the option to disable it was so that one could get the 'old'
behaviour back when I was initially adding LFS support, precisely
because it was causing issues. But that time is long past now, and I
don't see why anyone would want to say "please build me an nbd-server
that cannot read files with a size larger than 2GB".

Oh well.

[...]
> >>-		if (request.type==NBD_CMD_WRITE) {
> >>+		if ((request.type==NBD_CMD_WRITE) ||
> >>(request.type==NBD_CMD_WRITE_FUA)) {
> >
> >This makes me think that the _FUA thing should be something that can be
> >masked away; that way, we can do stuff like
> >
> >int type = request.type & ~NBD_CMD_FLAG_FUA;
> >if (type == NBD_CMD_WRITE) {
> >	...
> >}
> >
> >or something similar, which seems cleaner. It would also allow it to be
> >applied to other request types without having to special-case too much;
> >e.g., there's been talk of a truncate command, which could sparsify the
> >backend. I guess that could also use this FUA thing.
> 
> That's true. I considered doing it that way but I was attempting to make
> things as unintrusive as possible.

Which is a laudable goal, except if it gets in the way of code clarity,
as is the case here :-)

> I'm happy to recode this bit to use a FUA bit from the top of the
> command 32bit int. However, if we do that, we should probably reserve
> a fixed number of bits there (e.g.  16 bits - I can't imagine more
> than 65,536 different command types) for per-command flags).

Obviously, and 16 bits sounds like a good amount.

> We should then probably have a (separate) NBD_ flag that says "the
> server supports flags being passed in the the command type". It seemed
> a bit much to change just to save one command. What do you think?
> 
> Incidentally, I think that block could be better written as a switch
> on request.type. Assuming it is a read if it's not anything else
> is perhaps not the safest.

Yeah, absolutely; I'll be the first to admit that nbd-server is a bit
hairy in some places.

Originally it only supported the READ and WRITE commands, in which case
an if was reasonable; later, the DISC was added, and another if block
tacked along. As more protocol commands become available, however,
changing that to a switch is indeed the most sensible thing to do.

> >>-				}
> >>+				}	
> >
> >Another gratuitous whitespace change.
> 
> Fixed
> 
> >This file is taken from the kernel, so should probably be patched there
> >rather than here (though of course it would be hard to compile-test
> >without these bits).
> 
> Sure. Though I /think/ for proper compilation it needs to be fixed
> in both places, yes?

Sure :)

The point was merely that this file is copied verbatim from the kernel
source; whenever there's a difference between kernel/include/linux/nbd.h
and nbd/nbd.h, the former is supposed to be right, not the latter.

But those are details.

> >Please also document the new config file options in nbd-server.5.in.sgml.
> 
> Fixed
> 
> >Other than that, seems fine to me.
> 
> Thanks. New version attached, and at:
> 
> http://www.alex.org.uk/nbd-flush-fua-02.patch
> 
> Again, untested beyond compilation.
> 
> I'll have a look at doing the client and some testing.

Okay; I'll hold off on applying this until that's been done.

If relevant and possible, I'd appreciate it if you could also add
something to nbd-tester-client and the simple_test script, so that I
don't accidentally break it in the future. But don't sweat if it turns
out to be too hard.

-- 
The volume of a pizza of thickness a and radius z can be described by
the following formula:

pi zz a