On Wed, 2024-04-17 at 10:43 +1000, NeilBrown wrote: > On Wed, 17 Apr 2024, Jeff Layton wrote: > > On Wed, 2024-04-17 at 09:05 +1000, NeilBrown wrote: > > > On Wed, 17 Apr 2024, Jeff Layton wrote: > > > > On Wed, 2024-04-17 at 07:48 +1000, NeilBrown wrote: > > > > > On Tue, 16 Apr 2024, Jeff Layton wrote: > > > > > > On Tue, 2024-04-16 at 13:16 +1000, NeilBrown wrote: > > > > > > > On Tue, 16 Apr 2024, Lorenzo Bianconi wrote: > > > > > > > > Introduce write_version netlink command through a "declarative" interface. > > > > > > > > This patch introduces a change in behavior since for version-set userspace > > > > > > > > is expected to provide a NFS major/minor version list it wants to enable > > > > > > > > while all the other ones will be disabled. (procfs write_version > > > > > > > > command implements imperative interface where the admin writes +3/-3 to > > > > > > > > enable/disable a single version. > > > > > > > > > > > > > > It seems a little weird to me that the interface always disables all > > > > > > > version, but then also allows individual versions to be disabled. > > > > > > > > > > > > > > Would it be reasonable to simply ignore the "enabled" flag when setting > > > > > > > version, and just enable all versions listed?? > > > > > > > > > > > > > > Or maybe only enable those with the flag, and don't disable those > > > > > > > without the flag? > > > > > > > > > > > > > > Those don't necessarily seem much better - but the current behaviour > > > > > > > still seems odd. > > > > > > > > > > > > > > > > > > > I think it makes sense. > > > > > > > > > > > > We disable all versions, and enable any that have the "enabled" flag set > > > > > > in the call from userland. Userland technically needn't send down the > > > > > > versions that are disabled in the call, but the current userland program > > > > > > does. > > > > > > > > > > > > I worry about imperative interfaces that might only say -- "enable v4.1, > > > > > > but disable v3" and leave the others in their current state. That > > > > > > requires that both the kernel and userland keep state about what > > > > > > versions are currently enabled and disabled, and it's possible to get > > > > > > that wrong. > > > > > > > > > > I understand and support your aversion for imperative interfaces. > > > > > But this interface, as currently implemented, looks somewhat imperative. > > > > > The message sent to the kernel could enable some interfaces and then > > > > > disable them. I know that isn't the intent, but it is what the code > > > > > supports. Hence "weird". > > > > > > > > > > We could add code to make that sort of thing impossible, but there isn't > > > > > much point. Better to make it syntactically impossible. > > > > > > > > > > Realistically there will never be NFSv4.3 as there is no need - new > > > > > features can be added incrementally. > > > > > > > > > > > > > There is no need _so_far_. Who knows what the future will bring? Maybe > > > > we'll need v4.3 in order to add some needed feature? > > > > > > > > > So we could just pass an array of > > > > > 5 active flags: 2,3,4.0,4.1,4.2. I suspect you wouldn't like that and > > > > > I'm not sure that I do either. A "read" would return the same array > > > > > with 3 possible states: unavailable, disabled, enabled. (Maybe the > > > > > array could be variable length so 5.0 could be added one day...). > > > > > > > > > > > > > A set of flags is basically what this interface is. They just happen to > > > > also be labeled with the major and minorversion. I think that's a good > > > > thing. > > > > > > Good for whom? Labelling allows for labelling inconsistencies. > > > > > > > Now you're just being silly. You wanted a variable length array, but > > what if the next slot is not v4.3 but 5.0? A positional interpretation > > of a slot in an array is just as just as subject to problems. > > I don't think it could look like a imperative interface, which the > current one does a bit. > > > > > > Maybe the kernel should be able to provide an ordered list of labels, > > > and separately an array of which labels are enabled/disabled. > > > Userspace could send down a new set of enabled/disabled flags based on > > > the agreed set of labels. > > > > > > > How is this better than what's been proposed? One strength of netlink is > > that the data is structured. The already proposed interface takes > > advantage of that. > > > > > Here is a question that is a bit of a diversion, but might help us > > > understand the context more fully: > > > > > > Why would anyone disable v4.2 separately from v4.1 ?? > > > > > > > Furthermore, what does it mean to disable v4.1 but leave v4.2 enabled? > > Indeed! > > > > > > I understand that v2, v3, v4.0, v4.1 are effectively different protocols > > > and you might want to ensure that only the approved/tested protocol is > > > used. But v4.2 is just a few enhancements on v4.1. Why would you want > > > to disable it? > > > > > > The answer I can think of that there might be bugs (perish the > > > thought!!) in some of those features so you might want to avoid using > > > them. > > > But in that case, it is really the features that you want to suppress, > > > not the protocol version. > > > > > > Maybe I might want to disable delegation - to just write delegation. > > > Can I do that? What if I just want to disable server-side copy, but > > > keep fallocate and umask support? > > > > > > i.e. is a list of versions really the granularity that we want to use > > > for this interface? > > > > > > > Our current goal is to replace rpc.nfsd with a new program that works > > via netlink. An important bit of what rpc.nfsd does is start the NFS > > server with the settings in /etc/nfs.conf. Some of those settings are > > vers3=, vers4.0=, etc. that govern how /proc/fs/nfsd/versions is set. > > We have an immediate need to deal with those settings today, and > > probably will for quite some time. > > > > I'm not opposed to augmenting that with something more granular, but I > > don't think we should block this interface and wait on that. We can > > extend the interface at some point in the future to take a new feature > > bitmask or something, and just declare that (e.g.) declaring vers4.2=n > > disables some subset of those features. > > I agree that we don't want to block "good" while we wait for "perfect". I > just want to be sure that what we have is "good". > > The current /proc interface uses strings like "v3" and "v4.1". > The proposed netlink interface uses pairs on u32s - "major" and "minor". > So we lose some easy paths for extensibility. Are we comfortable with > that? > > This isn't a big deal - certainly not a blocked. I just don't feel > entirely comfortable about the current interface and I'm exploring to > see if there might be something better. > > Ok, I'm not convinced that anything you've proposed so far is better than what we have, but I'm open-minded. At this point we have these to-dos: 1) make the threads interface accept an array of integers rather than a singleton. This is needed when sunrpc.pool_mode is not global. 2) have the interface pass down the scope string in the THREADS_SET instead of making userland change the UTS namespace before starting threads. This should just mean adding a new optional string attribute to the threads interfaces. ...anything else we're missing that needs doing here? We'd really like to see this in -next soon, so we can possibly make v6.10 with this. Thanks, -- Jeff Layton <jlayton@xxxxxxxxxx>
