Re: [PATCH v2 0/5] rework access to /proc/net/rpc

Re: [PATCH v2 0/5] rework access to /proc/net/rpc

[David Härdeman]

Hi,

it seems that the "rework access to /proc/net/rpc" patchset removed 
dynamic buffers in favour of static, fixed size, buffers. That seems 
like a step backwards to me?

At least the readline() function could be implemented using read/write 
(instead of fread/fwrite) and a dynamic buffer...no?

//David

Re: [PATCH v2 0/5] rework access to /proc/net/rpc

[Timo Teras]

On Tue, 09 Dec 2014 09:16:59 +0100
David Härdeman <david@hardeman.nu> wrote:

> it seems that the "rework access to /proc/net/rpc" patchset removed 
> dynamic buffers in favour of static, fixed size, buffers. That seems 
> like a step backwards to me?

Depends a bit on your view. On read() side, readline() like
functionality is removed yes. Though, my understanding is so that this
is not needed with the kernel API. Maybe someone can correct me if I'm
wrong. The removal simplifies memory management, overall code size. As
probably has a positive impact on speed too (probably not too big, but
this communication is used all overall, so it might be useful).

On write() side the old code was completely wrong. It did several
assumptions on how FILE buffering works, most of them being incorrect
in general, but also glibc. It only worked because no large messages
have been sent to kernel.

> At least the readline() function could be implemented using
> read/write (instead of fread/fwrite) and a dynamic buffer...no?

It's extra complexity. I'd rather not add it unless it's required. My
understanding about the communication mechanism with kernel is that
it's not required. Why have code that would never be used?

Thanks,
Timo

Re: [PATCH v2 0/5] rework access to /proc/net/rpc

[David Härdeman]

On 2014-12-09 09:42, Timo Teras wrote:
> On Tue, 09 Dec 2014 09:16:59 +0100
> David Härdeman <david@hardeman.nu> wrote:
> 
>> it seems that the "rework access to /proc/net/rpc" patchset removed
>> dynamic buffers in favour of static, fixed size, buffers. That seems
>> like a step backwards to me?
> 
> Depends a bit on your view. On read() side, readline() like
> functionality is removed yes. Though, my understanding is so that this
> is not needed with the kernel API. Maybe someone can correct me if I'm
> wrong. The removal simplifies memory management, overall code size. As
> probably has a positive impact on speed too (probably not too big, but
> this communication is used all overall, so it might be useful).

And it makes the buffer size static, introducing an arbitrary limitation 
(although a rather large one...32KB allocated on the stack IIRC)

> On write() side the old code was completely wrong. It did several
> assumptions on how FILE buffering works, most of them being incorrect
> in general, but also glibc. It only worked because no large messages
> have been sent to kernel.

I didn't really check the write() side, it was just the readline() that 
I was interested in actually...

> 
>> At least the readline() function could be implemented using
>> read/write (instead of fread/fwrite) and a dynamic buffer...no?
> 
> It's extra complexity. I'd rather not add it unless it's required. My
> understanding about the communication mechanism with kernel is that
> it's not required. Why have code that would never be used?

I agree that it depends on your view. I tend to be very sceptical of 
arbitrary limitations unless they have a very good reason (like 
measurable and relevant performance impact), I doubt that's the case 
here.

It's up to the maintainer though, I just wanted to point it out :)

Regards,
David

Re: [PATCH v2 0/5] rework access to /proc/net/rpc

[Steve Dickson]

On 12/09/2014 09:01 AM, David Härdeman wrote:
> On 2014-12-09 09:42, Timo Teras wrote:
>> On Tue, 09 Dec 2014 09:16:59 +0100
>> David Härdeman <david@hardeman.nu> wrote:
>>
>>> it seems that the "rework access to /proc/net/rpc" patchset removed
>>> dynamic buffers in favour of static, fixed size, buffers. That seems
>>> like a step backwards to me?
>>
>> Depends a bit on your view. On read() side, readline() like
>> functionality is removed yes. Though, my understanding is so that this
>> is not needed with the kernel API. Maybe someone can correct me if I'm
>> wrong. The removal simplifies memory management, overall code size. As
>> probably has a positive impact on speed too (probably not too big, but
>> this communication is used all overall, so it might be useful).
> 
> And it makes the buffer size static, introducing an arbitrary limitation (although a rather large one...32KB allocated on the stack IIRC)
> 
>> On write() side the old code was completely wrong. It did several
>> assumptions on how FILE buffering works, most of them being incorrect
>> in general, but also glibc. It only worked because no large messages
>> have been sent to kernel.
> 
> I didn't really check the write() side, it was just the readline() that I was interested in actually...
> 
>>
>>> At least the readline() function could be implemented using
>>> read/write (instead of fread/fwrite) and a dynamic buffer...no?
>>
>> It's extra complexity. I'd rather not add it unless it's required. My
>> understanding about the communication mechanism with kernel is that
>> it's not required. Why have code that would never be used?
> 
> I agree that it depends on your view. I tend to be very sceptical of arbitrary 
> limitations unless they have a very good reason (like measurable and relevant 
> performance impact), I doubt that's the case here.
Your skeptical-ability of arbitrary limitations has become very clear in 
the last few hours... ;-) I guess I'm indifferent about it... From reading
your gssd patch set, it is a bit more artful not to use fixed size buffers
but again, I'm indifferent... That said... if patches appear removing these
fixed buffers they definitely would be considered... 

> 
> It's up to the maintainer though, I just wanted to point it out :)
My understanding these patches were needed to make nfs-utils compatible with the musl c-library.
That is the case, correct? 

steved.

Re: [PATCH v2 0/5] rework access to /proc/net/rpc

[David Härdeman]

On Tue, Dec 09, 2014 at 11:08:39AM -0500, Steve Dickson wrote:
>On 12/09/2014 09:01 AM, David Härdeman wrote:
>> On 2014-12-09 09:42, Timo Teras wrote:
>>> On Tue, 09 Dec 2014 09:16:59 +0100
>>> David Härdeman <david@hardeman.nu> wrote:
>>>> At least the readline() function could be implemented using
>>>> read/write (instead of fread/fwrite) and a dynamic buffer...no?
>>>
>>> It's extra complexity. I'd rather not add it unless it's required. My
>>> understanding about the communication mechanism with kernel is that
>>> it's not required. Why have code that would never be used?
>> 
>> I agree that it depends on your view. I tend to be very sceptical of arbitrary 
>> limitations unless they have a very good reason (like measurable and relevant 
>> performance impact), I doubt that's the case here.
>Your skeptical-ability of arbitrary limitations has become very clear in 
>the last few hours... ;-) I guess I'm indifferent about it... From reading
>your gssd patch set, it is a bit more artful not to use fixed size buffers
>but again, I'm indifferent... That said... if patches appear removing these
>fixed buffers they definitely would be considered... 
>
>> 
>> It's up to the maintainer though, I just wanted to point it out :)
>My understanding these patches were needed to make nfs-utils compatible with the musl c-library.
>That is the case, correct? 

The fread/fwrite removal seems reasonable, yes. The removal of the
readline() function though (which could be implemented using normal
read/malloc/realloc) seems less so.....IMHO.

-- 
David Härdeman

Re: [PATCH v2 0/5] rework access to /proc/net/rpc

[Steve Dickson]

On 12/09/2014 03:26 PM, David Härdeman wrote:
> On Tue, Dec 09, 2014 at 11:08:39AM -0500, Steve Dickson wrote:
>> On 12/09/2014 09:01 AM, David Härdeman wrote:
>>> On 2014-12-09 09:42, Timo Teras wrote:
>>>> On Tue, 09 Dec 2014 09:16:59 +0100
>>>> David Härdeman <david@hardeman.nu> wrote:
>>>>> At least the readline() function could be implemented using
>>>>> read/write (instead of fread/fwrite) and a dynamic buffer...no?
>>>>
>>>> It's extra complexity. I'd rather not add it unless it's required. My
>>>> understanding about the communication mechanism with kernel is that
>>>> it's not required. Why have code that would never be used?
>>>
>>> I agree that it depends on your view. I tend to be very sceptical of arbitrary 
>>> limitations unless they have a very good reason (like measurable and relevant 
>>> performance impact), I doubt that's the case here.
>> Your skeptical-ability of arbitrary limitations has become very clear in 
>> the last few hours... ;-) I guess I'm indifferent about it... From reading
>> your gssd patch set, it is a bit more artful not to use fixed size buffers
>> but again, I'm indifferent... That said... if patches appear removing these
>> fixed buffers they definitely would be considered... 
>>
>>>
>>> It's up to the maintainer though, I just wanted to point it out :)
>> My understanding these patches were needed to make nfs-utils compatible with the musl c-library.
>> That is the case, correct? 
> 
> The fread/fwrite removal seems reasonable, yes. The removal of the
> readline() function though (which could be implemented using normal
> read/malloc/realloc) seems less so.....IMHO.
> 
Patches welcome! 8-)

steved.

Re: [PATCH v2 0/5] rework access to /proc/net/rpc

[Timo Teras]

On Tue, 09 Dec 2014 16:30:52 -0500
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 12/09/2014 03:26 PM, David Härdeman wrote:
> > On Tue, Dec 09, 2014 at 11:08:39AM -0500, Steve Dickson wrote:
> >> On 12/09/2014 09:01 AM, David Härdeman wrote:
> >>> On 2014-12-09 09:42, Timo Teras wrote:
> >>>> On Tue, 09 Dec 2014 09:16:59 +0100
> >>>> David Härdeman <david@hardeman.nu> wrote:
> >>>>> At least the readline() function could be implemented using
> >>>>> read/write (instead of fread/fwrite) and a dynamic buffer...no?
> >>>>
> >>>> It's extra complexity. I'd rather not add it unless it's
> >>>> required. My understanding about the communication mechanism
> >>>> with kernel is that it's not required. Why have code that would
> >>>> never be used?
> >>>
> >>> I agree that it depends on your view. I tend to be very sceptical
> >>> of arbitrary limitations unless they have a very good reason
> >>> (like measurable and relevant performance impact), I doubt that's
> >>> the case here.
> >> Your skeptical-ability of arbitrary limitations has become very
> >> clear in the last few hours... ;-) I guess I'm indifferent about
> >> it... From reading your gssd patch set, it is a bit more artful
> >> not to use fixed size buffers but again, I'm indifferent... That
> >> said... if patches appear removing these fixed buffers they
> >> definitely would be considered... 
> >>
> >>>
> >>> It's up to the maintainer though, I just wanted to point it out :)
> >> My understanding these patches were needed to make nfs-utils
> >> compatible with the musl c-library. That is the case, correct? 

Yes. It is because musl FILE implementation uses writev() and readv()
with multiple buffers, and the kernel side does not handle that.

Also, the write side was very brittle even with glibc, it probably was
broken with large messages.

> > The fread/fwrite removal seems reasonable, yes. The removal of the
> > readline() function though (which could be implemented using normal
> > read/malloc/realloc) seems less so.....IMHO.
> > 
> Patches welcome! 8-)

Normally having upper limits is also a security feature. It means the
other end cannot force the client code to receive so large messages
that it runs out of memory. Though, in this case the other end is
kernel and to be trusted.

Though, it probably helps catch potential errors. The messages from
kernel really cannot exceed that length. If they do, it's an error
anyway. The message structure pretty much ensures this.

In my opinion the dynamic allocation is a step backward, rather then
forwards. It adds potential failure (out of memory), is not required,
and it does not add any features either.

IMHO, "just because it used to be so" is a bad excuse. And it would
just cause additional code making harder to debug and easier to fail.
Why add complexity when it can be done simpler?

just my five cents,
Timo

Re: [PATCH v2 0/5] rework access to /proc/net/rpc

[David Härdeman]

On 2014-12-10 07:09, Timo Teras wrote:
> On Tue, 09 Dec 2014 16:30:52 -0500
> Steve Dickson <SteveD@redhat.com> wrote:
>>>> My understanding these patches were needed to make nfs-utils
>>>> compatible with the musl c-library. That is the case, correct?
> 
> Yes. It is because musl FILE implementation uses writev() and readv()
> with multiple buffers, and the kernel side does not handle that.

I should probably note in that case that my patches to gssd include a 
call to fscanf, I'm guessing that'd be a problem for you?

> In my opinion the dynamic allocation is a step backward, rather then
> forwards. It adds potential failure (out of memory), is not required,
> and it does not add any features either.
> 
> IMHO, "just because it used to be so" is a bad excuse. And it would
> just cause additional code making harder to debug and easier to fail.
> Why add complexity when it can be done simpler?

I think PATH_MAX is a good counter-example.

But I think we can at least agree that we're discussing coding style 
now, which is a bit like discussing Emacs vs vi, and I doubt we'll ever 
reach an agreement... :)

Regards,
David

[PATCH v2 0/5] rework access to /proc/net/rpc

[Timo Teräs]

Changes since the first send:
- split to five separate patches
- fixed a bug in cache_get_filehandle() that made result parsing not work
- fixed to check result of write() calls

The review mentioned my patches adding:
nfssvc.c:71:8: warning: ignoring return value of 'system', declared with attribute warn_unused_result [-Wunused-result]
nfssvc.c:325:9: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
but this does not make any sense: I'm only adding one #include there.
Is that perhaps uncovering some other issues?

I'm not sure if the NFSv3 related issues were caused by the cache_get_filehandle()
issue or not, so this still needs testing. Another potential cause is that the
kernel is sending to user land requests longer than RPC_CHAN_BUF_SIZE bytes, but
that does not seem likely.

Timo Teräs (5):
  Add string.h to source files that need it
  mountd: talk to kernel using file descriptors instead of FILE
  gssd: talk to kernel using file descriptors instead of FILE
  nfsexport: talk to kernel using file descriptors instead of FILE
  nfslib: remove now unused FILE helpers

 support/include/exportfs.h     |   1 +
 support/include/nfslib.h       |   7 -
 support/include/nfsrpc.h       |   1 +
 support/nfs/cacheio.c          | 111 +------------
 support/nfs/nfsexport.c        |  77 +++++----
 utils/gssd/gssd_proc.c         |   9 +-
 utils/gssd/svcgssd.h           |   2 +-
 utils/gssd/svcgssd_main_loop.c |   9 +-
 utils/gssd/svcgssd_proc.c      |  51 +++---
 utils/gssd/write_bytes.h       |   1 +
 utils/mountd/cache.c           | 343 ++++++++++++++++++++++-------------------
 utils/nfsd/nfssvc.c            |   1 +
 12 files changed, 270 insertions(+), 343 deletions(-)

-- 
2.1.2

Re: [PATCH v2 0/5] rework access to /proc/net/rpc

[Steve Dickson]

On 10/02/2014 09:41 AM, Timo Teräs wrote:
> Changes since the first send:
> - split to five separate patches
> - fixed a bug in cache_get_filehandle() that made result parsing not work
> - fixed to check result of write() calls
> 
> The review mentioned my patches adding:
> nfssvc.c:71:8: warning: ignoring return value of 'system', declared with attribute warn_unused_result [-Wunused-result]
> nfssvc.c:325:9: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
> but this does not make any sense: I'm only adding one #include there.
> Is that perhaps uncovering some other issues?
> 
> I'm not sure if the NFSv3 related issues were caused by the cache_get_filehandle()
> issue or not, so this still needs testing. Another potential cause is that the
> kernel is sending to user land requests longer than RPC_CHAN_BUF_SIZE bytes, but
> that does not seem likely.
> 
> Timo Teräs (5):
>   Add string.h to source files that need it
>   mountd: talk to kernel using file descriptors instead of FILE
>   gssd: talk to kernel using file descriptors instead of FILE
>   nfsexport: talk to kernel using file descriptors instead of FILE
>   nfslib: remove now unused FILE helpers
> 
>  support/include/exportfs.h     |   1 +
>  support/include/nfslib.h       |   7 -
>  support/include/nfsrpc.h       |   1 +
>  support/nfs/cacheio.c          | 111 +------------
>  support/nfs/nfsexport.c        |  77 +++++----
>  utils/gssd/gssd_proc.c         |   9 +-
>  utils/gssd/svcgssd.h           |   2 +-
>  utils/gssd/svcgssd_main_loop.c |   9 +-
>  utils/gssd/svcgssd_proc.c      |  51 +++---
>  utils/gssd/write_bytes.h       |   1 +
>  utils/mountd/cache.c           | 343 ++++++++++++++++++++++-------------------
>  utils/nfsd/nfssvc.c            |   1 +
>  12 files changed, 270 insertions(+), 343 deletions(-)
> 
Committed and tested!

steved.