fs: interface for directly reading/writing compressed data

This is essentially copy_struct_from_user() but for an iov_iter.

+ iov_iter_advance(i, usize);

I think that you may need it to be based on Al's series for that to work, which might be inconvenient, though.

One other non-code issue: particularly since you only handle a subset of the iov_iter cases, it would be nice to have an explanation for _why_ those particular cases.

IOW, have some trivial explanation for each of the cases. "iovec" is for regular read/write, what triggers the kvec and bvec cases?

But also, the other way around. Why doesn't the pipe case trigger? No splice support?

Pipe ones are strictly destinations - they can't be sources. So if you see it called for one of those, you've a bug.

Xarray ones are *not* - they can be sources, and that's missing here.

Much more unpleasant, though, is that this thing has hard dependency on nr_seg == 1 *AND* openly suggests the use of iov_iter_single_seg_count(), which is completely wrong. That sucker has some weird users left (as of #work.iov_iter), but all of them are actually due to API deficiencies and I very much hope to kill that thing off.

Why not simply add iov_iter_check_zeroes(), that would be called after copy_from_iter() and verified that all that's left in the iterator consists of zeroes? Then this copy_struct_from_...() would be trivial to express through those two. And check_zeroes would also be trivial, especially on top of #work.iov_iter. With no calls of iov_iter_advance() at all, while we are at it...

IDGI... Omar, what semantics do you really want from that primitive?

Quoted 18 lines · Al Viro

Pipe ones are strictly destinations - they can't be sources. So if you see it called for one of those, you've a bug.

Xarray ones are *not* - they can be sources, and that's missing here.

Much more unpleasant, though, is that this thing has hard dependency on nr_seg == 1 *AND* openly suggests the use of iov_iter_single_seg_count(), which is completely wrong. That sucker has some weird users left (as of #work.iov_iter), but all of them are actually due to API deficiencies and I very much hope to kill that thing off.

Why not simply add iov_iter_check_zeroes(), that would be called after copy_from_iter() and verified that all that's left in the iterator consists of zeroes? Then this copy_struct_from_...() would be trivial to express through those two. And check_zeroes would also be trivial, especially on top of #work.iov_iter. With no calls of iov_iter_advance() at all, while we are at it...

Quoted 1 line

IDGI... Omar, what semantics do you really want from that primitive?

And for pity sake, let's not do that EXPORT_SYMBOL_GPL() posturing there. If it's a sane general-purpose API, it doesn't matter who uses it; if it's not, it shouldn't be exported in the first place.

It can be implemented via the already exported primitives, so it's not as if we prevented anyone from doing an equivalent...

Quoted 10 lines · Linus Torvalds

I think that you may need it to be based on Al's series for that to work, which might be inconvenient, though.

One other non-code issue: particularly since you only handle a subset of the iov_iter cases, it would be nice to have an explanation for _why_ those particular cases.

IOW, have some trivial explanation for each of the cases. "iovec" is for regular read/write, what triggers the kvec and bvec cases?

Quoted 2 lines

But also, the other way around. Why doesn't the pipe case trigger? No splice support?

Pipe ones are strictly destinations - they can't be sources. So if you see it called for one of those, you've a bug.

Xarray ones are *not* - they can be sources, and that's missing here.

Much more unpleasant, though, is that this thing has hard dependency on nr_seg == 1 *AND* openly suggests the use of iov_iter_single_seg_count(), which is completely wrong. That sucker has some weird users left (as of #work.iov_iter), but all of them are actually due to API deficiencies and I very much hope to kill that thing off.

Why not simply add iov_iter_check_zeroes(), that would be called after copy_from_iter() and verified that all that's left in the iterator consists of zeroes? Then this copy_struct_from_...() would be trivial to express through those two. And check_zeroes would also be trivial, especially on top of #work.iov_iter. With no calls of iov_iter_advance() at all, while we are at it...

IDGI... Omar, what semantics do you really want from that primitive?

RWF_ENCODED is intended to be used like this:

	struct encoded_iov encoded_iov = {
		/* compression metadata */ ...
	};
	char compressed_data[] = ...;
	struct iovec iov[] = {
		{ &encoded_iov, sizeof(encoded_iov) },
		{ compressed_data, sizeof(compressed_data) },
	};
	pwritev2(fd, iov, 2, -1, RWF_ENCODED);

Basically, we squirrel away the compression metadata in the first element of the iovec array, and we use iov[0].iov_len so that we can support future extensions of struct encoded_iov in the style of copy_struct_from_user().

So this doesn't require nr_seg == 1. On the contrary, it's expected that the rest of the iovec has the compressed payload. And to support the copy_struct_from_user()-style versioning, we need to know the size of the struct encoded_iov that userspace gave us, which is the reason for the iov_iter_single_seg_count().

I know this interface isn't the prettiest. It started as a Btrfs-specific ioctl, but this approach was suggested as a way to avoid having a whole new I/O path:
https://lore.kernel.org/linux-fsdevel/[email protected]/
The copy_struct_from_iter() thing was proposed as a way to allow future extensions here:
https://lore.kernel.org/linux-btrfs/[email protected]/

Please let me know if you have any suggestions for how to improve this.

Quoted 12 lines · Omar Sandoval

RWF_ENCODED is intended to be used like this:

      struct encoded_iov encoded_iov = {
              /* compression metadata */ ...
      };
      char compressed_data[] = ...;
      struct iovec iov[] = {
              { &encoded_iov, sizeof(encoded_iov) },
              { compressed_data, sizeof(compressed_data) },
      };
      pwritev2(fd, iov, 2, -1, RWF_ENCODED);

Quoted 4 lines

Basically, we squirrel away the compression metadata in the first element of the iovec array, and we use iov[0].iov_len so that we can support future extensions of struct encoded_iov in the style of copy_struct_from_user().

Yecchhh...

Just put the size of the encoded part first and be done with that. Magical effect of the iovec sizes is a bloody bad idea.

Quoted 2 lines

Just put the size of the encoded part first and be done with that. Magical effect of the iovec sizes is a bloody bad idea.

That makes everything uglier and more complicated, honestly. Then you'd have to do it in _two_ operations ("get the size, then get the rest"), *AND* you'd have to worry about all the corner-cases (ie people putting the structure in pieces across multiple iov entries.

Huh? All corner cases are already taken care of by copy_from_iter{,_full}(). What I'm proposing is to have the size as a field in 'encoded' and do this

Quoted 4 lines · Al Viro

Huh? All corner cases are already taken care of by copy_from_iter{,_full}(). What I'm proposing is to have the size as a field in 'encoded' and do this

Hmm. Making it part of the structure does make it easier (also for the sending userspace side, that doesn't now have to create yet another iov or copy the structure or whatever).

Except your code doesn't actually handle the "smaller than expected" case correctly, since by the time it even checks for that, it will possibly already have failed. So you actually had a bug there - you can't use the "xyz_full()" version and get it right.

That's fixable.

Or do the same reverting thing that Al did, but with copy_from_iter() instead of copy_from_iter_full() and being careful with the copied count (which I'm not 100% sure I got correct here):

	size_t copied = copy_from_iter(&encoded, sizeof(encoded), &i);
	if (copied < offsetofend(struct encoded_iov, size))
		return -EFAULT;
	if (encoded.size > PAGE_SIZE)
		return -E2BIG;
	if (encoded.size < ENCODED_IOV_SIZE_VER0)
		return -EINVAL;
	if (encoded.size > sizeof(encoded)) {
		if (copied < sizeof(encoded)
			return -EFAULT;
		if (!iov_iter_check_zeroes(&i, encoded.size - sizeof(encoded))

			return -EINVAL;
	} else if (encoded.size < sizeof(encoded)) {
		// older than what we expect
		if (copied < encoded.size)
			return -EFAULT;
		iov_iter_revert(&i, copied - encoded.size);
		memset((void *)&encoded + encoded.size, 0, sizeof(encoded) - encoded.size);
	}    

Quoted 15 lines · Omar Sandoval

Or do the same reverting thing that Al did, but with copy_from_iter() instead of copy_from_iter_full() and being careful with the copied count (which I'm not 100% sure I got correct here):

	size_t copied = copy_from_iter(&encoded, sizeof(encoded), &i);
	if (copied < offsetofend(struct encoded_iov, size))
		return -EFAULT;
	if (encoded.size > PAGE_SIZE)
		return -E2BIG;
	if (encoded.size < ENCODED_IOV_SIZE_VER0)
		return -EINVAL;
	if (encoded.size > sizeof(encoded)) {
		if (copied < sizeof(encoded)
			return -EFAULT;
		if (!iov_iter_check_zeroes(&i, encoded.size - sizeof(encoded))

Quoted 8 lines

			return -EINVAL;
	} else if (encoded.size < sizeof(encoded)) {
		// older than what we expect
		if (copied < encoded.size)
			return -EFAULT;
		iov_iter_revert(&i, copied - encoded.size);
		memset((void *)&encoded + encoded.size, 0, sizeof(encoded) - encoded.size);
	}    

simpler than that, actually -

	copied = copy_from_iter(&encoded, sizeof(encoded), &i);
	if (unlikely(copied < sizeof(encoded))) {
		if (copied < offsetofend(struct encoded_iov, size) ||
		    copied < encoded.size)
			return iov_iter_count(i) ? -EFAULT : -EINVAL;
	}
	if (encoded.size > sizeof(encoded)) {
		if (!iov_iter_check_zeroes(&i, encoded.size - sizeof(encoded))
			return -EINVAL;
	} else if (encoded.size < sizeof(encoded)) {
		// copied can't be less than encoded.size here - otherwise
		// we'd have copied < sizeof(encoded) and the check above
		// would've buggered off
		iov_iter_revert(&i, copied - encoded.size);
		memset((void *)&encoded + encoded.size, 0, sizeof(encoded) - encoded.size);
	}

should do it.

Quoted 45 lines · Al Viro

Quoted 15 lines · Omar Sandoval

Or do the same reverting thing that Al did, but with copy_from_iter() instead of copy_from_iter_full() and being careful with the copied count (which I'm not 100% sure I got correct here):

	size_t copied = copy_from_iter(&encoded, sizeof(encoded), &i);
	if (copied < offsetofend(struct encoded_iov, size))
		return -EFAULT;
	if (encoded.size > PAGE_SIZE)
		return -E2BIG;
	if (encoded.size < ENCODED_IOV_SIZE_VER0)
		return -EINVAL;
	if (encoded.size > sizeof(encoded)) {
		if (copied < sizeof(encoded)
			return -EFAULT;
		if (!iov_iter_check_zeroes(&i, encoded.size - sizeof(encoded))

Quoted 8 lines

			return -EINVAL;
	} else if (encoded.size < sizeof(encoded)) {
		// older than what we expect
		if (copied < encoded.size)
			return -EFAULT;
		iov_iter_revert(&i, copied - encoded.size);
		memset((void *)&encoded + encoded.size, 0, sizeof(encoded) - encoded.size);
	}    

simpler than that, actually -

	copied = copy_from_iter(&encoded, sizeof(encoded), &i);
	if (unlikely(copied < sizeof(encoded))) {
		if (copied < offsetofend(struct encoded_iov, size) ||
		    copied < encoded.size)
			return iov_iter_count(i) ? -EFAULT : -EINVAL;
	}
	if (encoded.size > sizeof(encoded)) {
		if (!iov_iter_check_zeroes(&i, encoded.size - sizeof(encoded))
			return -EINVAL;
	} else if (encoded.size < sizeof(encoded)) {
		// copied can't be less than encoded.size here - otherwise
		// we'd have copied < sizeof(encoded) and the check above
		// would've buggered off
		iov_iter_revert(&i, copied - encoded.size);
		memset((void *)&encoded + encoded.size, 0, sizeof(encoded) - encoded.size);
	}

Quoted 1 line

should do it.

Thanks, Al, I'll send an updated version with this approach next week.

How do we get the userspace size with the encoded_iov.size approach? We'd have to read the size from the iov_iter before writing to the rest of the iov_iter. Is it okay to mix the iov_iter as a source and destination like this? From what I can tell, it's not intended to be used like this.

Quoted 6 lines · Omar Sandoval

How do we get the userspace size with the encoded_iov.size approach? We'd have to read the size from the iov_iter before writing to the rest of the iov_iter. Is it okay to mix the iov_iter as a source and destination like this? From what I can tell, it's not intended to be used like this.

I guess it could work that way, but yes, it's ugly as hell. And I really don't want a readv() system call - that should write to the result buffer - to first have to read from it.

So I think the original "just make it be the first iov entry" is the better approach, even if Al hates it.

Although I still get the feeling that using an ioctl is the *really* correct way to go. That was my first reaction to the series originally, and I still don't see why we'd have encoded data in a regular read/write path.

What was the argument against ioctl's, again?

Quoted 15 lines · Linus Torvalds

Quoted 6 lines · Omar Sandoval

How do we get the userspace size with the encoded_iov.size approach? We'd have to read the size from the iov_iter before writing to the rest of the iov_iter. Is it okay to mix the iov_iter as a source and destination like this? From what I can tell, it's not intended to be used like this.

I guess it could work that way, but yes, it's ugly as hell. And I really don't want a readv() system call - that should write to the result buffer - to first have to read from it.

So I think the original "just make it be the first iov entry" is the better approach, even if Al hates it.

Quoted 6 lines

Although I still get the feeling that using an ioctl is the *really* correct way to go. That was my first reaction to the series originally, and I still don't see why we'd have encoded data in a regular read/write path.

What was the argument against ioctl's, again?

The suggestion came from Dave Chinner here:
https://lore.kernel.org/linux-fsdevel/[email protected]/

His objection to an ioctl was two-fold:

1. This interfaces looks really similar to normal read/write, so we should try to use the normal read/write interface for it. Perhaps this trouble with iov_iter has refuted that.
2. The last time we had Btrfs-specific ioctls that eventually became generic (FIDEDUPERANGE and FICLONE{,RANGE}), the generalization was painful. Part of the problem with clone/dedupe was that the Btrfs ioctls were underspecified. I think I've done a better job of documenting all of the semantics and corner cases for the encoded I/O interface (and if not, I can address this). The other part of the problem is that there were various sanity checks in the normal read/write paths that were missed or drifted out of sync in the ioctls. That requires some vigilance going forward. Maybe starting this off as a generic (not Btrfs-specific) ioctl right off the bat will help.

If we do go the ioctl route, then we also have to decide how much of preadv2/pwritev2 it should emulate. Should it use the fd offset, or should that be an ioctl argument? Some of the RWF_ flags would be useful for encoded I/O, too (RWF_DSYNC, RWF_SYNC, RWF_APPEND), should it support those? These bring us back to Dave's first point.

Quoted 43 lines · Omar Sandoval

Quoted 15 lines · Linus Torvalds

Quoted 6 lines · Omar Sandoval

How do we get the userspace size with the encoded_iov.size approach? We'd have to read the size from the iov_iter before writing to the rest of the iov_iter. Is it okay to mix the iov_iter as a source and destination like this? From what I can tell, it's not intended to be used like this.

I guess it could work that way, but yes, it's ugly as hell. And I really don't want a readv() system call - that should write to the result buffer - to first have to read from it.

So I think the original "just make it be the first iov entry" is the better approach, even if Al hates it.

Quoted 6 lines

Although I still get the feeling that using an ioctl is the *really* correct way to go. That was my first reaction to the series originally, and I still don't see why we'd have encoded data in a regular read/write path.

What was the argument against ioctl's, again?

The suggestion came from Dave Chinner here:
https://lore.kernel.org/linux-fsdevel/[email protected]/

His objection to an ioctl was two-fold:

1. This interfaces looks really similar to normal read/write, so we should try to use the normal read/write interface for it. Perhaps this trouble with iov_iter has refuted that.
2. The last time we had Btrfs-specific ioctls that eventually became generic (FIDEDUPERANGE and FICLONE{,RANGE}), the generalization was painful. Part of the problem with clone/dedupe was that the Btrfs ioctls were underspecified. I think I've done a better job of documenting all of the semantics and corner cases for the encoded I/O interface (and if not, I can address this). The other part of the problem is that there were various sanity checks in the normal read/write paths that were missed or drifted out of sync in the ioctls. That requires some vigilance going forward. Maybe starting this off as a generic (not Btrfs-specific) ioctl right off the bat will help.

Quoted 5 lines

If we do go the ioctl route, then we also have to decide how much of preadv2/pwritev2 it should emulate. Should it use the fd offset, or should that be an ioctl argument? Some of the RWF_ flags would be useful for encoded I/O, too (RWF_DSYNC, RWF_SYNC, RWF_APPEND), should it support those? These bring us back to Dave's first point.

Oops, I dropped Dave from the Cc list at some point. Adding him back now.

Quoted 44 lines · Omar Sandoval

Quoted 43 lines · Omar Sandoval

Quoted 15 lines · Linus Torvalds

Quoted 6 lines · Omar Sandoval

How do we get the userspace size with the encoded_iov.size approach? We'd have to read the size from the iov_iter before writing to the rest of the iov_iter. Is it okay to mix the iov_iter as a source and destination like this? From what I can tell, it's not intended to be used like this.

I guess it could work that way, but yes, it's ugly as hell. And I really don't want a readv() system call - that should write to the result buffer - to first have to read from it.

So I think the original "just make it be the first iov entry" is the better approach, even if Al hates it.

Quoted 6 lines

Although I still get the feeling that using an ioctl is the *really* correct way to go. That was my first reaction to the series originally, and I still don't see why we'd have encoded data in a regular read/write path.

What was the argument against ioctl's, again?

The suggestion came from Dave Chinner here:
https://lore.kernel.org/linux-fsdevel/[email protected]/

His objection to an ioctl was two-fold:

1. This interfaces looks really similar to normal read/write, so we should try to use the normal read/write interface for it. Perhaps this trouble with iov_iter has refuted that.
2. The last time we had Btrfs-specific ioctls that eventually became generic (FIDEDUPERANGE and FICLONE{,RANGE}), the generalization was painful. Part of the problem with clone/dedupe was that the Btrfs ioctls were underspecified. I think I've done a better job of documenting all of the semantics and corner cases for the encoded I/O interface (and if not, I can address this). The other part of the problem is that there were various sanity checks in the normal read/write paths that were missed or drifted out of sync in the ioctls. That requires some vigilance going forward. Maybe starting this off as a generic (not Btrfs-specific) ioctl right off the bat will help.

Quoted 8 lines

Quoted 5 lines

If we do go the ioctl route, then we also have to decide how much of preadv2/pwritev2 it should emulate. Should it use the fd offset, or should that be an ioctl argument? Some of the RWF_ flags would be useful for encoded I/O, too (RWF_DSYNC, RWF_SYNC, RWF_APPEND), should it support those? These bring us back to Dave's first point.

Oops, I dropped Dave from the Cc list at some point. Adding him back now.

Fair summary. The only other thing that I'd add is this is an IO interface that requires issuing physical IO. So if someone wants high throughput for encoded IO, we really need AIO and/or io_uring support, and we get that for free if we use readv2/writev2 interfaces.

Yes, it could be an ioctl() interface, but I think that this sort of functionality is exactly what extensible syscalls like preadv2/pwritev2 should be used for. It's a slight variant on normal IO, and that's exactly what the RWF_* flags are intended to be used for - allowing interesting per-IO variant behaviour without having to completely re-implemnt the IO path via custom ioctls every time we want slightly different functionality...

Al, Linus, what do you think? Is there a path forward for this series as is?

I'd be happy to have this functionality merged in any form, but I do
think that this approach with preadv2/pwritev2 using iov_len is decent relative to the alternatives.

Quoted 3 lines · Omar Sandoval

Al, Linus, what do you think? Is there a path forward for this series as is?

So the "read from user space in order to write" is a no-go for me. It completely violates what a "read()" system call should do. It also entirely violates what an iovec can and should do.

And honestly, if Al hates the "first iov entry" model, I'm not sure I want to merge that version - I personally find it fine, but Al is effectively the iov-iter maintainer.

I do worry a bit about the "first iov entry" simply because it might work for "writev2()" when given virtual user space addresses - but I think it's conceptually broken for things like direct-IO which might do things by physical address, and what is a contiguous user space virtual address is not necessarily a contiguous physical address.

Yes, the filesystem can - and does - hide that path by basically not doing direct-IO on the first entry at all, and just treat is very specially in the front end of the IO access, but that only reinforces the whole "this is not at all like read/write".

Similar issues might crop up in other situations, ie splice etc, where it's not at all obvious that the iov_iter boundaries would be maintained as it moves through the system.

So while I personally find the "first iov entry" model fairly reasonable, I think Dave is being disingenuous when he says that it looks like a normal read/write. It very much does not. The above is quite fundamental.

Quoted 3 lines

I'd be happy to have this functionality merged in any form, but I do
think that this approach with preadv2/pwritev2 using iov_len is decent relative to the alternatives.

As mentioned, I find it acceptable. I'm completely unimpressed with Dave's argument, but ioctl's aren't perfect either, so weak or not, that argument being bogus doesn't necessarily mean that the iovec entry model is wrong.

That said, thinking about exactly the fact that I don't think a translation from iovec to anything else can be truly valid, I find the iter_is_iovec() case to be the only obviously valid one.

Which gets me back to: how can any of the non-iovec alternatives ever be valid? You did mention having missed ITER_XARRAY, but my question is more fundamental than that. How could a non-iter_is_iovec ever be valid? There are no possible interfaces that can generate such a thing sanely.

Quoted 6 lines

Fair summary. The only other thing that I'd add is this is an IO interface that requires issuing physical IO. So if someone wants high throughput for encoded IO, we really need AIO and/or io_uring support, and we get that for free if we use readv2/writev2 interfaces.

Quoted 7 lines

Yes, it could be an ioctl() interface, but I think that this sort of functionality is exactly what extensible syscalls like preadv2/pwritev2 should be used for. It's a slight variant on normal IO, and that's exactly what the RWF_* flags are intended to be used for - allowing interesting per-IO variant behaviour without having to completely re-implemnt the IO path via custom ioctls every time we want slightly different functionality...

Al, Linus, what do you think? Is there a path forward for this series as is? I'd be happy to have this functionality merged in any form, but I do think that this approach with preadv2/pwritev2 using iov_len is decent relative to the alternatives.

Quoted 14 lines · Omar Sandoval

Quoted 6 lines

Fair summary. The only other thing that I'd add is this is an IO interface that requires issuing physical IO. So if someone wants high throughput for encoded IO, we really need AIO and/or io_uring support, and we get that for free if we use readv2/writev2 interfaces.

Quoted 7 lines

Yes, it could be an ioctl() interface, but I think that this sort of functionality is exactly what extensible syscalls like preadv2/pwritev2 should be used for. It's a slight variant on normal IO, and that's exactly what the RWF_* flags are intended to be used for - allowing interesting per-IO variant behaviour without having to completely re-implemnt the IO path via custom ioctls every time we want slightly different functionality...

Quoted 4 lines

Al, Linus, what do you think? Is there a path forward for this series as is? I'd be happy to have this functionality merged in any form, but I do think that this approach with preadv2/pwritev2 using iov_len is decent relative to the alternatives.

IMO we might be better off with explicit ioctl - this magical mystery shite with special meaning of the first iovec length is, IMO, more than enough to make it a bad fit for read/write family.

It's *not* just a "slightly different functionality" - it's very different calling conventions. And the deeper one needs to dig into the interface details to parse what's going on, the less it differs from ioctl() mess.

Said that, why do you need a variable-length header on the read side, in the first place?

Suppose we add a new field representing a new type of encoding to the end of encoded_iov. On the write side, the caller might want to specify that the data is encoded in that new way, of course. But on the read side, if the data is encoded in that new way, then the kernel will want to return that. The kernel needs to know if the user's structure includes the new field (otherwise when it copies the full struct out, it will write into what the user thinks is the data instead).

Quoted 7 lines · Omar Sandoval

Suppose we add a new field representing a new type of encoding to the end of encoded_iov. On the write side, the caller might want to specify that the data is encoded in that new way, of course. But on the read side, if the data is encoded in that new way, then the kernel will want to return that. The kernel needs to know if the user's structure includes the new field (otherwise when it copies the full struct out, it will write into what the user thinks is the data instead).

Er... What's the problem with simply copying that extended structure out, followed by the data?

IOW, why can't the caller pick the header out of the whole thing and deal with it in whatever way it likes? Why should kernel need to do anything special here?

IDGI... Userland had always been able to deal with that kind of stuff; you read e.g. gzipped data into buffer, you decode the header, you figure out how long it is and how far out does the payload begin, etc.

How is that different?

Ah, I was stuck on thinking about this calling convention:

	struct encoded_iov encoded_iov;
	char compressed_data[...];
	struct iovec iov[] = {
		{ &encoded_iov, sizeof(encoded_iov) },
		{ compressed_data, sizeof(compressed_data) },
	};
	preadv2(fd, iov, 2, -1, RWF_ENCODED);

But what you described would look more like:

	// Needs to be large enough for maximum returned header + data.
	char buffer[...];
	struct iovec iov[] = {
		{ buffer, sizeof(buffer) },
	};
	preadv2(fd, iov, 2, -1, RWF_ENCODED);
	// We should probably align the buffer.
	struct encoded_iov *encoded_iov = (void *)buffer;
	char *data = buffer + encoded_iov->size;

That's a little uglier, but it should work, and allows for arbitrary extensions. So, among these three alternatives (fixed size structure with reserved space, variable size structure like above, or ioctl), which would you prefer?

But what you described would look more like:

	// Needs to be large enough for maximum returned header + data.
	char buffer[...];
	struct iovec iov[] = {
		{ buffer, sizeof(buffer) },
	};
	preadv2(fd, iov, 2, -1, RWF_ENCODED);
	// We should probably align the buffer.
	struct encoded_iov *encoded_iov = (void *)buffer;
	char *data = buffer + encoded_iov->size;

That's a little uglier, but it should work, and allows for arbitrary extensions. So, among these three alternatives (fixed size structure with reserved space, variable size structure like above, or ioctl), which would you prefer?

Quoted 12 lines · Omar Sandoval

But what you described would look more like:

	// Needs to be large enough for maximum returned header + data.
	char buffer[...];
	struct iovec iov[] = {
		{ buffer, sizeof(buffer) },
	};
	preadv2(fd, iov, 2, -1, RWF_ENCODED);
	// We should probably align the buffer.
	struct encoded_iov *encoded_iov = (void *)buffer;
	char *data = buffer + encoded_iov->size;

Quoted 4 lines

That's a little uglier, but it should work, and allows for arbitrary extensions. So, among these three alternatives (fixed size structure with reserved space, variable size structure like above, or ioctl), which would you prefer?

Does that work for O_DIRECT and the required 512-byte alignment?

Quoted 2 lines · Matthew Wilcox

Does that work for O_DIRECT and the required 512-byte alignment?

I suppose the kernel could pad the encoded_iov structure with zeroes to the next sector boundary, since zeroes are effectively noops for encoded_iov.

Quoted 8 lines · Omar Sandoval

Quoted 2 lines · Matthew Wilcox

Does that work for O_DIRECT and the required 512-byte alignment?

I suppose the kernel could pad the encoded_iov structure with zeroes to the next sector boundary, since zeroes are effectively noops for encoded_iov.

Ugh.

I really think the whole "embed the control structure in the stream" is wrong. The alignment issue is just another sign of that.

Separating it out is the right thing to do. At least the "first iov entry" thing did separate the control structure from the actual data. I detest the whole "embed the two together".

I'll suggest the fixed-size struct encoded_iov again, then. If we're willing to give up some of the flexibility of a variable size, then userspace can always put the fixed-size structure in its own iovec or include it inline with the data, depending on what's more convenient and whether it's using O_DIRECT.

I also worry that this "raw compressed data" thing isn't the only thing people will want to do. I could easily see some kind of "end-to-end CRC read/write" where the user passes in not just the data, but also checksums for it to validate it (maybe because you're doing a file copy and had the original checksums, but also maybe because user space simply has a known good copy and doesn't want errors re-introduced due to memory corruption).

Linus,

Quoted 7 lines

I also worry that this "raw compressed data" thing isn't the only thing people will want to do. I could easily see some kind of "end-to-end CRC read/write" where the user passes in not just the data, but also checksums for it to validate it (maybe because you're doing a file copy and had the original checksums, but also maybe because user space simply has a known good copy and doesn't want errors re-introduced due to memory corruption).

We already support passing CRCs down to be validated by the hardware for both NVMe and SCSI. This currently only works from the block layer down. When enabled, the checksums are generated by the block layer for writes and the data is validated against the checksums sent by the storage on reads.

Over the years various attempts at adding support for passing the checksum buffers in from userland have failed for exactly the reasons outlined in this thread (Joel, Darrick, Bob). Would love to have a generic way of passing this kind of information...

Does it make any kind of sense to talk about doing this for buffered I/O, given that we can't generate them for (eg) mmaped files?

Quoted 3 lines · Matthew Wilcox

Does it make any kind of sense to talk about doing this for buffered I/O, given that we can't generate them for (eg) mmaped files?

Sure we can.

Or rather, some people might very well like to do it even for mutable data. In fact, _especially_ for mutable data.

You might want to do things like "write out the state I verified just a moment ago", and if it has changed since then, you *want* the result to be invalid because the checksums no longer match - in case somebody else changed the data you used for the state calculation and verification in the meantime. It's very much why you'd want a separate checksum in the first place.

Yeah, yeah, you can - and people do - just do things like this with a separate checksum. But if you know that the filesystem has internal checksumming support _anyway_, you might want to use it, and basically say "use this checksum, if the data doesn't match when I read it back I want to get an IO error".

(The "data doesn't match" _could_ be just due to DRAM corruption etc, of course. Some people care about things like that. You want "verified" filesystem contents - it might not be about security, it might simply be about "I have validated this data and if it's not the same data any more it's useless and I need to re-generate it").

Am I a big believer in this model? No. Portability concerns (across OS'es, across filesystems, even just across backups on the same exact system) means that even if we did this, very few people would use it.

People who want this end up using an external checksum instead and do it outside of and separately from the actual IO, because then they can do it on existing systems.

So my argument is not "we want this". My argument is purely that some buffered filesystem IO case isn't actually any different from the traditional "I want access to the low-level sector hardware checksum data". The use cases are basically exactly the same.

Of course, basically nobody does that hw sector checksum either, for all the same reasons, even if it's been around for decades.

So my "checksum metadata interface" is not something I'm a big believer in, but I really don't think it's really all _that_ different from the whole "compressed format interface" that this whole patch series is about. They are pretty much the same thing in many ways.

Quoted 42 lines · Linus Torvalds

Quoted 3 lines · Matthew Wilcox

Does it make any kind of sense to talk about doing this for buffered I/O, given that we can't generate them for (eg) mmaped files?

Sure we can.

Or rather, some people might very well like to do it even for mutable data. In fact, _especially_ for mutable data.

You might want to do things like "write out the state I verified just a moment ago", and if it has changed since then, you *want* the result to be invalid because the checksums no longer match - in case somebody else changed the data you used for the state calculation and verification in the meantime. It's very much why you'd want a separate checksum in the first place.

Yeah, yeah, you can - and people do - just do things like this with a separate checksum. But if you know that the filesystem has internal checksumming support _anyway_, you might want to use it, and basically say "use this checksum, if the data doesn't match when I read it back I want to get an IO error".

(The "data doesn't match" _could_ be just due to DRAM corruption etc, of course. Some people care about things like that. You want "verified" filesystem contents - it might not be about security, it might simply be about "I have validated this data and if it's not the same data any more it's useless and I need to re-generate it").

Am I a big believer in this model? No. Portability concerns (across OS'es, across filesystems, even just across backups on the same exact system) means that even if we did this, very few people would use it.

People who want this end up using an external checksum instead and do it outside of and separately from the actual IO, because then they can do it on existing systems.

So my argument is not "we want this". My argument is purely that some buffered filesystem IO case isn't actually any different from the traditional "I want access to the low-level sector hardware checksum data". The use cases are basically exactly the same.

Quoted 7 lines

Of course, basically nobody does that hw sector checksum either, for all the same reasons, even if it's been around for decades.

So my "checksum metadata interface" is not something I'm a big believer in, but I really don't think it's really all _that_ different from the whole "compressed format interface" that this whole patch series is about. They are pretty much the same thing in many ways.

I see the similarity in the sense that we basically want to pass some extra metadata down with the read or write. So then do we want to add preadv3/pwritev3 for encoded I/O now so that checksums can use it in the future? The encoding metadata could go in this "struct io_how", either directly or in a separate structure with a pointer in "struct io_how". It could get messy with compat syscalls.

Quoted 51 lines · Omar Sandoval

Quoted 42 lines · Linus Torvalds

Quoted 3 lines · Matthew Wilcox

Does it make any kind of sense to talk about doing this for buffered I/O, given that we can't generate them for (eg) mmaped files?

Sure we can.

Or rather, some people might very well like to do it even for mutable data. In fact, _especially_ for mutable data.

You might want to do things like "write out the state I verified just a moment ago", and if it has changed since then, you *want* the result to be invalid because the checksums no longer match - in case somebody else changed the data you used for the state calculation and verification in the meantime. It's very much why you'd want a separate checksum in the first place.

Yeah, yeah, you can - and people do - just do things like this with a separate checksum. But if you know that the filesystem has internal checksumming support _anyway_, you might want to use it, and basically say "use this checksum, if the data doesn't match when I read it back I want to get an IO error".

(The "data doesn't match" _could_ be just due to DRAM corruption etc, of course. Some people care about things like that. You want "verified" filesystem contents - it might not be about security, it might simply be about "I have validated this data and if it's not the same data any more it's useless and I need to re-generate it").

Am I a big believer in this model? No. Portability concerns (across OS'es, across filesystems, even just across backups on the same exact system) means that even if we did this, very few people would use it.

People who want this end up using an external checksum instead and do it outside of and separately from the actual IO, because then they can do it on existing systems.

So my argument is not "we want this". My argument is purely that some buffered filesystem IO case isn't actually any different from the traditional "I want access to the low-level sector hardware checksum data". The use cases are basically exactly the same.

Quoted 7 lines

Of course, basically nobody does that hw sector checksum either, for all the same reasons, even if it's been around for decades.

So my "checksum metadata interface" is not something I'm a big believer in, but I really don't think it's really all _that_ different from the whole "compressed format interface" that this whole patch series is about. They are pretty much the same thing in many ways.

Quoted 6 lines

I see the similarity in the sense that we basically want to pass some extra metadata down with the read or write. So then do we want to add preadv3/pwritev3 for encoded I/O now so that checksums can use it in the future? The encoding metadata could go in this "struct io_how", either directly or in a separate structure with a pointer in "struct io_how". It could get messy with compat syscalls.

Ping. What's the path forward here? At this point, it seems like an ioctl is the path of least resistance.

I'm also really worried with overloading the regular r/w path and iov_iter with ever more special cases. We already have various performance problems in the path, and adding more special cases ain't gonna help.

Quoted 4 lines · Al Viro

Huh? All corner cases are already taken care of by copy_from_iter{,_full}(). What I'm proposing is to have the size as a field in 'encoded' and do this

Hmm. Making it part of the structure does make it easier (also for the sending userspace side, that doesn't now have to create yet another iov or copy the structure or whatever).

Except your code doesn't actually handle the "smaller than expected" case correctly, since by the time it even checks for that, it will possibly already have failed. So you actually had a bug there - you can't use the "xyz_full()" version and get it right.