[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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



Wouter,

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.

Sure.

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".

Well, on the principle of least change, what I am doing now is
ignoring HAVE_SYNC_FILE_RANGE (and thus avoiding setting _GNU_SOURCE)
unless we are compiling with NBD_LFS. As sync_file_range is a 64
bit call, I would not like to bet on it working well in combination with
non-LFS builds. This means that if you are building without NBD_LFS
you get fdatasync() instead, which is not the end of the world.

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.

OK, I will recode it to do that.

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.

OK, I might refactor that then.

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

That is wise :-)

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.

Yes, I'll look at that too.

--
Alex Bligh



Reply to: