efivarfs and writev() support

efivarfs and writev() support

[Marcel Holtmann]

Hi,

it seems that efivarfs only supports readv(), but when it comes to writev(), I am getting an error. Is there any reason to not support vectored write on this filesystem? Especially with the uint32 header for each file, I think it would make perfect sense to support it.

Regards

Marcel

Re: efivarfs and writev() support

[Matt Fleming]

On Sun, 08 Mar, at 03:24:09PM, Marcel Holtmann wrote:
> Hi,
> 
> it seems that efivarfs only supports readv(), but when it comes to
> writev(), I am getting an error. Is there any reason to not support
> vectored write on this filesystem? Especially with the uint32 header
> for each file, I think it would make perfect sense to support it.

What error are you seeing? I thought that the vfs fell back to a looped
write if the file system doesn't support .write_iter()?

But yes, we definitely should support writev().

-- 
Matt Fleming, Intel Open Source Technology Center

Re: efivarfs and writev() support

[Marcel Holtmann]

Hi Matt,

>> it seems that efivarfs only supports readv(), but when it comes to
>> writev(), I am getting an error. Is there any reason to not support
>> vectored write on this filesystem? Especially with the uint32 header
>> for each file, I think it would make perfect sense to support it.
> 
> What error are you seeing? I thought that the vfs fell back to a looped
> write if the file system doesn't support .write_iter()?

that seems to work for readv(), but not for writev().

> But yes, we definitely should support writev().

I just get an EIO error and have not traced this down any further.

Regards

Marcel

Re: efivarfs and writev() support

[Al Viro]

On Wed, Mar 11, 2015 at 08:12:52AM -0700, Marcel Holtmann wrote:
> Hi Matt,
> 
> >> it seems that efivarfs only supports readv(), but when it comes to
> >> writev(), I am getting an error. Is there any reason to not support
> >> vectored write on this filesystem? Especially with the uint32 header
> >> for each file, I think it would make perfect sense to support it.
> > 
> > What error are you seeing? I thought that the vfs fell back to a looped
> > write if the file system doesn't support .write_iter()?
> 
> that seems to work for readv(), but not for writev().
> 
> > But yes, we definitely should support writev().
> 
> I just get an EIO error and have not traced this down any further.

What arguments are you feeding to it?  Note that the thing is sensitive to
range boundaries; it's not as if series of write() to it would be equivalent
to single write() from concatenation.  And writev() is equivalent to
series of write().

If you want behaviour a-la UDP sockets (syscall boundaries matter,
boundaries between vector elements do not), we can certainly do that,
but this is different from the current semantics.  AFAICS, said
semantics makes little sense, but it's a user-visible change...

Re: efivarfs and writev() support

[Marcel Holtmann]

Hi Al,

>>>> it seems that efivarfs only supports readv(), but when it comes to
>>>> writev(), I am getting an error. Is there any reason to not support
>>>> vectored write on this filesystem? Especially with the uint32 header
>>>> for each file, I think it would make perfect sense to support it.
>>> 
>>> What error are you seeing? I thought that the vfs fell back to a looped
>>> write if the file system doesn't support .write_iter()?
>> 
>> that seems to work for readv(), but not for writev().
>> 
>>> But yes, we definitely should support writev().
>> 
>> I just get an EIO error and have not traced this down any further.
> 
> What arguments are you feeding to it?  Note that the thing is sensitive to
> range boundaries; it's not as if series of write() to it would be equivalent
> to single write() from concatenation.  And writev() is equivalent to
> series of write().

I did something really simple and from my point obvious. I took the uint32 header that every file needs and put that in iov[0] pointer and then the rest in iov[1] pointer. The reason was that I didn't want to copy the actual file content around to just add a uint32 header in front of it.

> If you want behaviour a-la UDP sockets (syscall boundaries matter,
> boundaries between vector elements do not), we can certainly do that,
> but this is different from the current semantics.  AFAICS, said
> semantics makes little sense, but it's a user-visible change...

I do not know about the specific semantics of efivarfs and frankly I have not tried every single combination. However it sounds to me that currently it requires that the whole file content is provided with a single write(). I have no idea if this is true or not. I do not know enough about the internals here.

Maybe efivarfs just needs to implemented .write_iter properly to actually support writev() and can not rely on a fallback of multiple write() calls.

Regards

Marcel

Re: efivarfs and writev() support

[Al Viro]

On Thu, Mar 12, 2015 at 07:58:35AM -0700, Marcel Holtmann wrote:

> I do not know about the specific semantics of efivarfs and frankly I have not tried every single combination. However it sounds to me that currently it requires that the whole file content is provided with a single write(). I have no idea if this is true or not. I do not know enough about the internals here.
> 
> Maybe efivarfs just needs to implemented .write_iter properly to actually support writev() and can not rely on a fallback of multiple write() calls.

Sigh...  There are three variants of write/writev semantics:
	1) stream.  Neither syscall nor vector member boundaries matter,
the thing on the other end of IO channel might interpret the stream of
data it's being fed and carve it into pieces, but that's a function of
the contents.  TCP sockets are like that, so are pipes, etc.
	2) syscall-level datagram.  Vector member boundaries do not matter,
syscall ones do.  UDP is like that - iovec is pure scatter-gather thing
there; the boundaries come from syscalls.
	3) vector-level datagram.  Each vector member represents a single
datagram, syscall boundaries do not matter.  I.e. iovec is an array of
datagrams.  Most of character devices are like that.  And so's efivarfs.

What you are proposing seems to be switching it to syscall-level datagram
behaviour.  It's very unlikely to break anything (I would be very surprised
if anything tried to use "send this array of datagrams", simply because it's
usually[1] bloody pointless for those files), but it *is* a user-visible API
change.

And if we go for it, sure, we should just switch to ->write_iter() and be
done with that - kmalloc(iov_iter_count(to), GFP_KERNEL), copy_from_iter(),
use the first 4 bytes for attributes and the rest for body, same as we do
now.

[1] not always - variable doesn't have to have "each time we set it, the
old value is completely lost" semantics, even though most of them are
that way.

Re: efivarfs and writev() support

[Marcel Holtmann]

Hi Al,

>> I do not know about the specific semantics of efivarfs and frankly I have not tried every single combination. However it sounds to me that currently it requires that the whole file content is provided with a single write(). I have no idea if this is true or not. I do not know enough about the internals here.
>> 
>> Maybe efivarfs just needs to implemented .write_iter properly to actually support writev() and can not rely on a fallback of multiple write() calls.
> 
> Sigh...  There are three variants of write/writev semantics:
> 	1) stream.  Neither syscall nor vector member boundaries matter,
> the thing on the other end of IO channel might interpret the stream of
> data it's being fed and carve it into pieces, but that's a function of
> the contents.  TCP sockets are like that, so are pipes, etc.
> 	2) syscall-level datagram.  Vector member boundaries do not matter,
> syscall ones do.  UDP is like that - iovec is pure scatter-gather thing
> there; the boundaries come from syscalls.
> 	3) vector-level datagram.  Each vector member represents a single
> datagram, syscall boundaries do not matter.  I.e. iovec is an array of
> datagrams.  Most of character devices are like that.  And so's efivarfs.
> 
> What you are proposing seems to be switching it to syscall-level datagram
> behaviour.  It's very unlikely to break anything (I would be very surprised
> if anything tried to use "send this array of datagrams", simply because it's
> usually[1] bloody pointless for those files), but it *is* a user-visible API
> change.
> 
> And if we go for it, sure, we should just switch to ->write_iter() and be
> done with that - kmalloc(iov_iter_count(to), GFP_KERNEL), copy_from_iter(),
> use the first 4 bytes for attributes and the rest for body, same as we do
> now.

I say we should support writev() on efivarfs. Not supporting it seems odd especially since that is not documented anywhere. So yes, I am for adding .write_iter() support and be done with that.

Also please note that even write(,4) and write(,n) does not work either. You can not write partial entries as it seems. Maybe you are able to append, but it seems the initial creation of the variable has to be done with a single write() call. Anything else ends up in a file with 0 length.

Regards

Marcel

Re: efivarfs and writev() support

[Matt Fleming]

On Sat, 14 Mar, at 09:33:00AM, Marcel Holtmann wrote:
> 
> I say we should support writev() on efivarfs. Not supporting it seems
> odd especially since that is not documented anywhere. So yes, I am for
> adding .write_iter() support and be done with that.

Well, as Al has explained it's not that writev() isn't supported, it's
that having an iovec vector containing only the 4-byte variable
attribute isn't currently supported.

Since writev() calls are intended to be atomic, and even though efivarfs
gets fed the variable data in chunks we can process it in one go, I
think allowing the scenario you describe is fine.

> Also please note that even write(,4) and write(,n) does not work
> either. You can not write partial entries as it seems. Maybe you are
> able to append, but it seems the initial creation of the variable has
> to be done with a single write() call. Anything else ends up in a file
> with 0 length.

Yes, that's by design. I guess it's to prohibit people from creating
bogus EFI variables or accidentally deleting variables (a SetVariable()
call with length 0 is a delete).

-- 
Matt Fleming, Intel Open Source Technology Center

Re: efivarfs and writev() support

[Marcel Holtmann]

Hi Matt,

>> I say we should support writev() on efivarfs. Not supporting it seems
>> odd especially since that is not documented anywhere. So yes, I am for
>> adding .write_iter() support and be done with that.
> 
> Well, as Al has explained it's not that writev() isn't supported, it's
> that having an iovec vector containing only the 4-byte variable
> attribute isn't currently supported.
> 
> Since writev() calls are intended to be atomic, and even though efivarfs
> gets fed the variable data in chunks we can process it in one go, I
> think allowing the scenario you describe is fine.

meaning only writev() with a single vector is supported.

>> Also please note that even write(,4) and write(,n) does not work
>> either. You can not write partial entries as it seems. Maybe you are
>> able to append, but it seems the initial creation of the variable has
>> to be done with a single write() call. Anything else ends up in a file
>> with 0 length.
> 
> Yes, that's by design. I guess it's to prohibit people from creating
> bogus EFI variables or accidentally deleting variables (a SetVariable()
> call with length 0 is a delete).

I have no problems with these semantics, but it means you need to add support for .write_iter since otherwise multiple vectors will not work. And really think that is better than forcing userspace to work around this limitation.

As I mentioned earlier, for me it was really obvious to use a vectored write for EFI variables. That just made sense to me. So seeing it not working caused confusion.

Regards

Marcel