[BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Victor Stewart]

just auto updated from 5.13.16 to 5.13.17, and suddenly my fixed
file registrations fail with EOPNOTSUPP using liburing 2.0.

static inline struct io_uring ring;
static inline int *socketfds;

// ...

void enableFD(int fd)
{
   int result = io_uring_register_files_update(&ring, fd,
                      &(socketfds[fd] = fd), 1);
   printf("enableFD, result = %d\n", result);
}

maybe this is due to the below and related work that
occurred at the end of 5.13 and liburing got out of sync?

https://github.com/torvalds/linux/commit/992da01aa932b432ef8dc3885fa76415b5dbe43f#diff-79ffab63f24ef28eec3badbc8769e2a23e0475ab1fbe390207269ece944a0824

and can't use liburing 2.1 because of the api changes since 5.13.

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Jens Axboe]

On 9/18/21 7:41 AM, Victor Stewart wrote:
> just auto updated from 5.13.16 to 5.13.17, and suddenly my fixed
> file registrations fail with EOPNOTSUPP using liburing 2.0.
> 
> static inline struct io_uring ring;
> static inline int *socketfds;
> 
> // ...
> 
> void enableFD(int fd)
> {
>    int result = io_uring_register_files_update(&ring, fd,
>                       &(socketfds[fd] = fd), 1);
>    printf("enableFD, result = %d\n", result);
> }
> 
> maybe this is due to the below and related work that
> occurred at the end of 5.13 and liburing got out of sync?
> 
> https://github.com/torvalds/linux/commit/992da01aa932b432ef8dc3885fa76415b5dbe43f#diff-79ffab63f24ef28eec3badbc8769e2a23e0475ab1fbe390207269ece944a0824
> 
> and can't use liburing 2.1 because of the api changes since 5.13.

That's very strange, the -EOPNOTSUPP should only be possible if you
are not passing in the ring fd for the register syscall. You should
be able to mix and match liburing versions just fine, the only exception
is sometimes between releases (of both liburing and the kernel) where we
have the liberty to change the API of something that was added before
release.

Can you do an strace of it and attach?

-- 
Jens Axboe

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Victor Stewart]

On Sat, Sep 18, 2021 at 3:41 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 9/18/21 7:41 AM, Victor Stewart wrote:
> > just auto updated from 5.13.16 to 5.13.17, and suddenly my fixed
> > file registrations fail with EOPNOTSUPP using liburing 2.0.
> >
> > static inline struct io_uring ring;
> > static inline int *socketfds;
> >
> > // ...
> >
> > void enableFD(int fd)
> > {
> >    int result = io_uring_register_files_update(&ring, fd,
> >                       &(socketfds[fd] = fd), 1);
> >    printf("enableFD, result = %d\n", result);
> > }
> >
> > maybe this is due to the below and related work that
> > occurred at the end of 5.13 and liburing got out of sync?
> >
> > https://github.com/torvalds/linux/commit/992da01aa932b432ef8dc3885fa76415b5dbe43f#diff-79ffab63f24ef28eec3badbc8769e2a23e0475ab1fbe390207269ece944a0824
> >
> > and can't use liburing 2.1 because of the api changes since 5.13.
>
> That's very strange, the -EOPNOTSUPP should only be possible if you
> are not passing in the ring fd for the register syscall. You should
> be able to mix and match liburing versions just fine, the only exception
> is sometimes between releases (of both liburing and the kernel) where we
> have the liberty to change the API of something that was added before
> release.
>
> Can you do an strace of it and attach?

oh ya the EOPNOTSUPP was my bug introduced trying to debug.

here's the real bug...

io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7,
8, 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
-1, -1, -1, -1, -1,
-1, ...], 32768) = -1 EMFILE (Too many open files)

32,768 is 1U << 15 aka IORING_MAX_FIXED_FILES, but i tried
16,000 just to try and same issue.

maybe you're not allowed to have pre-filled (aka non negative 1)
entries upon the initial io_uring_register_files call anymore?

this was working until the 5.13.16 -> 5.13.17 transition.
>
> --
> Jens Axboe
>

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Jens Axboe]

On 9/18/21 2:13 PM, Victor Stewart wrote:
> On Sat, Sep 18, 2021 at 3:41 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 9/18/21 7:41 AM, Victor Stewart wrote:
>>> just auto updated from 5.13.16 to 5.13.17, and suddenly my fixed
>>> file registrations fail with EOPNOTSUPP using liburing 2.0.
>>>
>>> static inline struct io_uring ring;
>>> static inline int *socketfds;
>>>
>>> // ...
>>>
>>> void enableFD(int fd)
>>> {
>>>    int result = io_uring_register_files_update(&ring, fd,
>>>                       &(socketfds[fd] = fd), 1);
>>>    printf("enableFD, result = %d\n", result);
>>> }
>>>
>>> maybe this is due to the below and related work that
>>> occurred at the end of 5.13 and liburing got out of sync?
>>>
>>> https://github.com/torvalds/linux/commit/992da01aa932b432ef8dc3885fa76415b5dbe43f#diff-79ffab63f24ef28eec3badbc8769e2a23e0475ab1fbe390207269ece944a0824
>>>
>>> and can't use liburing 2.1 because of the api changes since 5.13.
>>
>> That's very strange, the -EOPNOTSUPP should only be possible if you
>> are not passing in the ring fd for the register syscall. You should
>> be able to mix and match liburing versions just fine, the only exception
>> is sometimes between releases (of both liburing and the kernel) where we
>> have the liberty to change the API of something that was added before
>> release.
>>
>> Can you do an strace of it and attach?
> 
> oh ya the EOPNOTSUPP was my bug introduced trying to debug.
> 
> here's the real bug...
> 
> io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7,
> 8, 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> -1, -1, -1, -1, -1,
> -1, ...], 32768) = -1 EMFILE (Too many open files)
> 
> 32,768 is 1U << 15 aka IORING_MAX_FIXED_FILES, but i tried
> 16,000 just to try and same issue.
> 
> maybe you're not allowed to have pre-filled (aka non negative 1)
> entries upon the initial io_uring_register_files call anymore?
> 
> this was working until the 5.13.16 -> 5.13.17 transition.

Ah yes that makes more sense. You need to up RLIMIT_NOFILE, the
registered files are under that protection now too. This is also why it
was brought back to stable. A bit annoying, but it was needed for the
direct file support to have some sanity there.

So use rlimit(RLIMIT_NOFILE,...) from the app or ulimit -n to bump the
limit.

-- 
Jens Axboe

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Jens Axboe]

On Sat, Sep 18, 2021 at 2:26 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 9/18/21 2:13 PM, Victor Stewart wrote:
> > On Sat, Sep 18, 2021 at 3:41 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>
> >> On 9/18/21 7:41 AM, Victor Stewart wrote:
> >>> just auto updated from 5.13.16 to 5.13.17, and suddenly my fixed
> >>> file registrations fail with EOPNOTSUPP using liburing 2.0.
> >>>
> >>> static inline struct io_uring ring;
> >>> static inline int *socketfds;
> >>>
> >>> // ...
> >>>
> >>> void enableFD(int fd)
> >>> {
> >>>    int result = io_uring_register_files_update(&ring, fd,
> >>>                       &(socketfds[fd] = fd), 1);
> >>>    printf("enableFD, result = %d\n", result);
> >>> }
> >>>
> >>> maybe this is due to the below and related work that
> >>> occurred at the end of 5.13 and liburing got out of sync?
> >>>
> >>> https://github.com/torvalds/linux/commit/992da01aa932b432ef8dc3885fa76415b5dbe43f#diff-79ffab63f24ef28eec3badbc8769e2a23e0475ab1fbe390207269ece944a0824
> >>>
> >>> and can't use liburing 2.1 because of the api changes since 5.13.
> >>
> >> That's very strange, the -EOPNOTSUPP should only be possible if you
> >> are not passing in the ring fd for the register syscall. You should
> >> be able to mix and match liburing versions just fine, the only exception
> >> is sometimes between releases (of both liburing and the kernel) where we
> >> have the liberty to change the API of something that was added before
> >> release.
> >>
> >> Can you do an strace of it and attach?
> >
> > oh ya the EOPNOTSUPP was my bug introduced trying to debug.
> >
> > here's the real bug...
> >
> > io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7,
> > 8, 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> > -1, -1, -1, -1, -1,
> > -1, ...], 32768) = -1 EMFILE (Too many open files)
> >
> > 32,768 is 1U << 15 aka IORING_MAX_FIXED_FILES, but i tried
> > 16,000 just to try and same issue.
> >
> > maybe you're not allowed to have pre-filled (aka non negative 1)
> > entries upon the initial io_uring_register_files call anymore?
> >
> > this was working until the 5.13.16 -> 5.13.17 transition.
>
> Ah yes that makes more sense. You need to up RLIMIT_NOFILE, the
> registered files are under that protection now too. This is also why it
> was brought back to stable. A bit annoying, but it was needed for the
> direct file support to have some sanity there.
>
> So use rlimit(RLIMIT_NOFILE,...) from the app or ulimit -n to bump the
> limit.

BTW, this could be incorporated into io_uring_register_files and
io_uring_register_files_tags(), might not be a bad idea in general. Just
have it check rlim.rlim_cur for RLIMIT_NOFILE, and if it's smaller than
'nr_files', then bump it. That'd hide it nicely, instead of throwing a
failure.

-- 
Jens Axboe

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Victor Stewart]

On Sat, Sep 18, 2021 at 9:38 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On Sat, Sep 18, 2021 at 2:26 PM Jens Axboe <axboe@kernel.dk> wrote:
> >
> > On 9/18/21 2:13 PM, Victor Stewart wrote:
> > > On Sat, Sep 18, 2021 at 3:41 PM Jens Axboe <axboe@kernel.dk> wrote:
> > >>
> > >> On 9/18/21 7:41 AM, Victor Stewart wrote:
> > >>> just auto updated from 5.13.16 to 5.13.17, and suddenly my fixed
> > >>> file registrations fail with EOPNOTSUPP using liburing 2.0.
> > >>>
> > >>> static inline struct io_uring ring;
> > >>> static inline int *socketfds;
> > >>>
> > >>> // ...
> > >>>
> > >>> void enableFD(int fd)
> > >>> {
> > >>>    int result = io_uring_register_files_update(&ring, fd,
> > >>>                       &(socketfds[fd] = fd), 1);
> > >>>    printf("enableFD, result = %d\n", result);
> > >>> }
> > >>>
> > >>> maybe this is due to the below and related work that
> > >>> occurred at the end of 5.13 and liburing got out of sync?
> > >>>
> > >>> https://github.com/torvalds/linux/commit/992da01aa932b432ef8dc3885fa76415b5dbe43f#diff-79ffab63f24ef28eec3badbc8769e2a23e0475ab1fbe390207269ece944a0824
> > >>>
> > >>> and can't use liburing 2.1 because of the api changes since 5.13.
> > >>
> > >> That's very strange, the -EOPNOTSUPP should only be possible if you
> > >> are not passing in the ring fd for the register syscall. You should
> > >> be able to mix and match liburing versions just fine, the only exception
> > >> is sometimes between releases (of both liburing and the kernel) where we
> > >> have the liberty to change the API of something that was added before
> > >> release.
> > >>
> > >> Can you do an strace of it and attach?
> > >
> > > oh ya the EOPNOTSUPP was my bug introduced trying to debug.
> > >
> > > here's the real bug...
> > >
> > > io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7,
> > > 8, 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> > > -1, -1, -1, -1, -1,
> > > -1, ...], 32768) = -1 EMFILE (Too many open files)
> > >
> > > 32,768 is 1U << 15 aka IORING_MAX_FIXED_FILES, but i tried
> > > 16,000 just to try and same issue.
> > >
> > > maybe you're not allowed to have pre-filled (aka non negative 1)
> > > entries upon the initial io_uring_register_files call anymore?
> > >
> > > this was working until the 5.13.16 -> 5.13.17 transition.
> >
> > Ah yes that makes more sense. You need to up RLIMIT_NOFILE, the
> > registered files are under that protection now too. This is also why it
> > was brought back to stable. A bit annoying, but it was needed for the
> > direct file support to have some sanity there.
> >
> > So use rlimit(RLIMIT_NOFILE,...) from the app or ulimit -n to bump the
> > limit.
>

perfect got it working with..

struct rlimit maxFilesLimit = {N_IOURING_MAX_FIXED_FILES,
N_IOURING_MAX_FIXED_FILES};
setrlimit(RLIMIT_NOFILE, &maxFilesLimit);

> BTW, this could be incorporated into io_uring_register_files and
> io_uring_register_files_tags(), might not be a bad idea in general. Just
> have it check rlim.rlim_cur for RLIMIT_NOFILE, and if it's smaller than
> 'nr_files', then bump it. That'd hide it nicely, instead of throwing a
> failure.

the implicit bump sounds like a good idea (at least in theory?).

another thing i think might be a good idea is an io_uring
change/migration log that we update with every kernel release covering
new features but also new restrictions/requirements/tweaks etc.

something that would take 1 minute to skim and see if relevant.

because at this point to stay fully updated requires reading all of the
mailing list or checking pulls on your branch + running to binaries
to see if anything breaks.

>
> --
> Jens Axboe

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Jens Axboe]

On 9/18/21 3:55 PM, Victor Stewart wrote:
> On Sat, Sep 18, 2021 at 9:38 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On Sat, Sep 18, 2021 at 2:26 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 9/18/21 2:13 PM, Victor Stewart wrote:
>>>> On Sat, Sep 18, 2021 at 3:41 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>
>>>>> On 9/18/21 7:41 AM, Victor Stewart wrote:
>>>>>> just auto updated from 5.13.16 to 5.13.17, and suddenly my fixed
>>>>>> file registrations fail with EOPNOTSUPP using liburing 2.0.
>>>>>>
>>>>>> static inline struct io_uring ring;
>>>>>> static inline int *socketfds;
>>>>>>
>>>>>> // ...
>>>>>>
>>>>>> void enableFD(int fd)
>>>>>> {
>>>>>>    int result = io_uring_register_files_update(&ring, fd,
>>>>>>                       &(socketfds[fd] = fd), 1);
>>>>>>    printf("enableFD, result = %d\n", result);
>>>>>> }
>>>>>>
>>>>>> maybe this is due to the below and related work that
>>>>>> occurred at the end of 5.13 and liburing got out of sync?
>>>>>>
>>>>>> https://github.com/torvalds/linux/commit/992da01aa932b432ef8dc3885fa76415b5dbe43f#diff-79ffab63f24ef28eec3badbc8769e2a23e0475ab1fbe390207269ece944a0824
>>>>>>
>>>>>> and can't use liburing 2.1 because of the api changes since 5.13.
>>>>>
>>>>> That's very strange, the -EOPNOTSUPP should only be possible if you
>>>>> are not passing in the ring fd for the register syscall. You should
>>>>> be able to mix and match liburing versions just fine, the only exception
>>>>> is sometimes between releases (of both liburing and the kernel) where we
>>>>> have the liberty to change the API of something that was added before
>>>>> release.
>>>>>
>>>>> Can you do an strace of it and attach?
>>>>
>>>> oh ya the EOPNOTSUPP was my bug introduced trying to debug.
>>>>
>>>> here's the real bug...
>>>>
>>>> io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7,
>>>> 8, 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
>>>> -1, -1, -1, -1, -1,
>>>> -1, ...], 32768) = -1 EMFILE (Too many open files)
>>>>
>>>> 32,768 is 1U << 15 aka IORING_MAX_FIXED_FILES, but i tried
>>>> 16,000 just to try and same issue.
>>>>
>>>> maybe you're not allowed to have pre-filled (aka non negative 1)
>>>> entries upon the initial io_uring_register_files call anymore?
>>>>
>>>> this was working until the 5.13.16 -> 5.13.17 transition.
>>>
>>> Ah yes that makes more sense. You need to up RLIMIT_NOFILE, the
>>> registered files are under that protection now too. This is also why it
>>> was brought back to stable. A bit annoying, but it was needed for the
>>> direct file support to have some sanity there.
>>>
>>> So use rlimit(RLIMIT_NOFILE,...) from the app or ulimit -n to bump the
>>> limit.
>>
> 
> perfect got it working with..
> 
> struct rlimit maxFilesLimit = {N_IOURING_MAX_FIXED_FILES,
> N_IOURING_MAX_FIXED_FILES};
> setrlimit(RLIMIT_NOFILE, &maxFilesLimit);

Good!

>> BTW, this could be incorporated into io_uring_register_files and
>> io_uring_register_files_tags(), might not be a bad idea in general. Just
>> have it check rlim.rlim_cur for RLIMIT_NOFILE, and if it's smaller than
>> 'nr_files', then bump it. That'd hide it nicely, instead of throwing a
>> failure.
> 
> the implicit bump sounds like a good idea (at least in theory?).

Can you try current liburing -git? Remove your own RLIMIT_NOFILE and
just verify that it works. I pushed a change for it.

> another thing i think might be a good idea is an io_uring
> change/migration log that we update with every kernel release covering
> new features but also new restrictions/requirements/tweaks etc.

Yes, that is a good idea. The man pages do tend to reference what
version included what, but a highlight per release would be a great idea
to have without having to dig for it.

> something that would take 1 minute to skim and see if relevant.
> 
> because at this point to stay fully updated requires reading all of the
> mailing list or checking pulls on your branch + running to binaries
> to see if anything breaks.

Question is where to post it? Because I would post it here anyway...

-- 
Jens Axboe

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Victor Stewart]

On Sat, Sep 18, 2021 at 11:21 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 9/18/21 3:55 PM, Victor Stewart wrote:
> > On Sat, Sep 18, 2021 at 9:38 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>
> >> On Sat, Sep 18, 2021 at 2:26 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>
> >>> On 9/18/21 2:13 PM, Victor Stewart wrote:
> >>>> On Sat, Sep 18, 2021 at 3:41 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>>>
> >>>>> On 9/18/21 7:41 AM, Victor Stewart wrote:
> >>>>>> just auto updated from 5.13.16 to 5.13.17, and suddenly my fixed
> >>>>>> file registrations fail with EOPNOTSUPP using liburing 2.0.
> >>>>>>
> >>>>>> static inline struct io_uring ring;
> >>>>>> static inline int *socketfds;
> >>>>>>
> >>>>>> // ...
> >>>>>>
> >>>>>> void enableFD(int fd)
> >>>>>> {
> >>>>>>    int result = io_uring_register_files_update(&ring, fd,
> >>>>>>                       &(socketfds[fd] = fd), 1);
> >>>>>>    printf("enableFD, result = %d\n", result);
> >>>>>> }
> >>>>>>
> >>>>>> maybe this is due to the below and related work that
> >>>>>> occurred at the end of 5.13 and liburing got out of sync?
> >>>>>>
> >>>>>> https://github.com/torvalds/linux/commit/992da01aa932b432ef8dc3885fa76415b5dbe43f#diff-79ffab63f24ef28eec3badbc8769e2a23e0475ab1fbe390207269ece944a0824
> >>>>>>
> >>>>>> and can't use liburing 2.1 because of the api changes since 5.13.
> >>>>>
> >>>>> That's very strange, the -EOPNOTSUPP should only be possible if you
> >>>>> are not passing in the ring fd for the register syscall. You should
> >>>>> be able to mix and match liburing versions just fine, the only exception
> >>>>> is sometimes between releases (of both liburing and the kernel) where we
> >>>>> have the liberty to change the API of something that was added before
> >>>>> release.
> >>>>>
> >>>>> Can you do an strace of it and attach?
> >>>>
> >>>> oh ya the EOPNOTSUPP was my bug introduced trying to debug.
> >>>>
> >>>> here's the real bug...
> >>>>
> >>>> io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7,
> >>>> 8, 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> >>>> -1, -1, -1, -1, -1,
> >>>> -1, ...], 32768) = -1 EMFILE (Too many open files)
> >>>>
> >>>> 32,768 is 1U << 15 aka IORING_MAX_FIXED_FILES, but i tried
> >>>> 16,000 just to try and same issue.
> >>>>
> >>>> maybe you're not allowed to have pre-filled (aka non negative 1)
> >>>> entries upon the initial io_uring_register_files call anymore?
> >>>>
> >>>> this was working until the 5.13.16 -> 5.13.17 transition.
> >>>
> >>> Ah yes that makes more sense. You need to up RLIMIT_NOFILE, the
> >>> registered files are under that protection now too. This is also why it
> >>> was brought back to stable. A bit annoying, but it was needed for the
> >>> direct file support to have some sanity there.
> >>>
> >>> So use rlimit(RLIMIT_NOFILE,...) from the app or ulimit -n to bump the
> >>> limit.
> >>
> >
> > perfect got it working with..
> >
> > struct rlimit maxFilesLimit = {N_IOURING_MAX_FIXED_FILES,
> > N_IOURING_MAX_FIXED_FILES};
> > setrlimit(RLIMIT_NOFILE, &maxFilesLimit);
>
> Good!
>
> >> BTW, this could be incorporated into io_uring_register_files and
> >> io_uring_register_files_tags(), might not be a bad idea in general. Just
> >> have it check rlim.rlim_cur for RLIMIT_NOFILE, and if it's smaller than
> >> 'nr_files', then bump it. That'd hide it nicely, instead of throwing a
> >> failure.
> >
> > the implicit bump sounds like a good idea (at least in theory?).
>
> Can you try current liburing -git? Remove your own RLIMIT_NOFILE and
> just verify that it works. I pushed a change for it.

i don't have a dev box up right now, but i applied the below changes to 2.0
sans the tags bit...

diff --git a/src/register.c b/src/register.c
index 994aaff..495216a 100644
--- a/src/register.c
+++ b/src/register.c
@@ -7,6 +7,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <string.h>
+#include <sys/resource.h>

 #include "liburing/compat.h"
 #include "liburing/io_uring.h"
@@ -14,6 +15,22 @@

 #include "syscall.h"

+static int bump_rlimit_nofile(unsigned nr)
+{
+       struct rlimit rlim;
+
+       if (getrlimit(RLIMIT_NOFILE, &rlim) < 0)
+               return -errno;
+       if (rlim.rlim_cur < nr) {
+               if (nr > rlim.rlim_max)
+                       return -EMFILE;
+               rlim.rlim_cur = nr;
+               setrlimit(RLIMIT_NOFILE, &rlim);
+       }
+
+       return 0;
+}
+
 int io_uring_register_buffers(struct io_uring *ring, const struct
iovec *iovecs,
                              unsigned nr_iovecs)
 {
@@ -55,6 +72,10 @@ int io_uring_register_files_update(struct io_uring
*ring, unsigned off,
        };
        int ret;

+       ret = bump_rlimit_nofile(nr_files);
+       if (ret)
+               return ret;
+

and it failed with the same as before...

io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7, 8,
9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
-1, -1, -1, -1,
-1, ...], 32768) = -1 EMFILE (Too many open files)

if you want i can debug it for you tomorrow? (in london)

>
> > another thing i think might be a good idea is an io_uring
> > change/migration log that we update with every kernel release covering
> > new features but also new restrictions/requirements/tweaks etc.
>
> Yes, that is a good idea. The man pages do tend to reference what
> version included what, but a highlight per release would be a great idea
> to have without having to dig for it.
>
> > something that would take 1 minute to skim and see if relevant.
> >
> > because at this point to stay fully updated requires reading all of the
> > mailing list or checking pulls on your branch + running to binaries
> > to see if anything breaks.
>
> Question is where to post it? Because I would post it here anyway...

i think a txt file in liburing might be the perfect place given the audience
for it is solely application developers? could start with 5.15 and maintain
it forward.

>
> --
> Jens Axboe
>

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Victor Stewart]

On Sun, Sep 19, 2021 at 12:19 AM Victor Stewart <v@nametag.social> wrote:
>
> On Sat, Sep 18, 2021 at 11:21 PM Jens Axboe <axboe@kernel.dk> wrote:
> >
> > On 9/18/21 3:55 PM, Victor Stewart wrote:
> > > On Sat, Sep 18, 2021 at 9:38 PM Jens Axboe <axboe@kernel.dk> wrote:
> > >>
> > >> On Sat, Sep 18, 2021 at 2:26 PM Jens Axboe <axboe@kernel.dk> wrote:
> > >>>
> > >>> On 9/18/21 2:13 PM, Victor Stewart wrote:
> > >>>> On Sat, Sep 18, 2021 at 3:41 PM Jens Axboe <axboe@kernel.dk> wrote:
> > >>>>>
> > >>>>> On 9/18/21 7:41 AM, Victor Stewart wrote:
> > >>>>>> just auto updated from 5.13.16 to 5.13.17, and suddenly my fixed
> > >>>>>> file registrations fail with EOPNOTSUPP using liburing 2.0.
> > >>>>>>
> > >>>>>> static inline struct io_uring ring;
> > >>>>>> static inline int *socketfds;
> > >>>>>>
> > >>>>>> // ...
> > >>>>>>
> > >>>>>> void enableFD(int fd)
> > >>>>>> {
> > >>>>>>    int result = io_uring_register_files_update(&ring, fd,
> > >>>>>>                       &(socketfds[fd] = fd), 1);
> > >>>>>>    printf("enableFD, result = %d\n", result);
> > >>>>>> }
> > >>>>>>
> > >>>>>> maybe this is due to the below and related work that
> > >>>>>> occurred at the end of 5.13 and liburing got out of sync?
> > >>>>>>
> > >>>>>> https://github.com/torvalds/linux/commit/992da01aa932b432ef8dc3885fa76415b5dbe43f#diff-79ffab63f24ef28eec3badbc8769e2a23e0475ab1fbe390207269ece944a0824
> > >>>>>>
> > >>>>>> and can't use liburing 2.1 because of the api changes since 5.13.
> > >>>>>
> > >>>>> That's very strange, the -EOPNOTSUPP should only be possible if you
> > >>>>> are not passing in the ring fd for the register syscall. You should
> > >>>>> be able to mix and match liburing versions just fine, the only exception
> > >>>>> is sometimes between releases (of both liburing and the kernel) where we
> > >>>>> have the liberty to change the API of something that was added before
> > >>>>> release.
> > >>>>>
> > >>>>> Can you do an strace of it and attach?
> > >>>>
> > >>>> oh ya the EOPNOTSUPP was my bug introduced trying to debug.
> > >>>>
> > >>>> here's the real bug...
> > >>>>
> > >>>> io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7,
> > >>>> 8, 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> > >>>> -1, -1, -1, -1, -1,
> > >>>> -1, ...], 32768) = -1 EMFILE (Too many open files)
> > >>>>
> > >>>> 32,768 is 1U << 15 aka IORING_MAX_FIXED_FILES, but i tried
> > >>>> 16,000 just to try and same issue.
> > >>>>
> > >>>> maybe you're not allowed to have pre-filled (aka non negative 1)
> > >>>> entries upon the initial io_uring_register_files call anymore?
> > >>>>
> > >>>> this was working until the 5.13.16 -> 5.13.17 transition.
> > >>>
> > >>> Ah yes that makes more sense. You need to up RLIMIT_NOFILE, the
> > >>> registered files are under that protection now too. This is also why it
> > >>> was brought back to stable. A bit annoying, but it was needed for the
> > >>> direct file support to have some sanity there.
> > >>>
> > >>> So use rlimit(RLIMIT_NOFILE,...) from the app or ulimit -n to bump the
> > >>> limit.
> > >>
> > >
> > > perfect got it working with..
> > >
> > > struct rlimit maxFilesLimit = {N_IOURING_MAX_FIXED_FILES,
> > > N_IOURING_MAX_FIXED_FILES};
> > > setrlimit(RLIMIT_NOFILE, &maxFilesLimit);
> >
> > Good!
> >
> > >> BTW, this could be incorporated into io_uring_register_files and
> > >> io_uring_register_files_tags(), might not be a bad idea in general. Just
> > >> have it check rlim.rlim_cur for RLIMIT_NOFILE, and if it's smaller than
> > >> 'nr_files', then bump it. That'd hide it nicely, instead of throwing a
> > >> failure.
> > >
> > > the implicit bump sounds like a good idea (at least in theory?).
> >
> > Can you try current liburing -git? Remove your own RLIMIT_NOFILE and
> > just verify that it works. I pushed a change for it.
>
> i don't have a dev box up right now, but i applied the below changes to 2.0
> sans the tags bit...
>
> diff --git a/src/register.c b/src/register.c
> index 994aaff..495216a 100644
> --- a/src/register.c
> +++ b/src/register.c
> @@ -7,6 +7,7 @@
>  #include <unistd.h>
>  #include <errno.h>
>  #include <string.h>
> +#include <sys/resource.h>
>
>  #include "liburing/compat.h"
>  #include "liburing/io_uring.h"
> @@ -14,6 +15,22 @@
>
>  #include "syscall.h"
>
> +static int bump_rlimit_nofile(unsigned nr)
> +{
> +       struct rlimit rlim;
> +
> +       if (getrlimit(RLIMIT_NOFILE, &rlim) < 0)
> +               return -errno;
> +       if (rlim.rlim_cur < nr) {
> +               if (nr > rlim.rlim_max)
> +                       return -EMFILE;
> +               rlim.rlim_cur = nr;
> +               setrlimit(RLIMIT_NOFILE, &rlim);
> +       }
> +
> +       return 0;
> +}
> +
>  int io_uring_register_buffers(struct io_uring *ring, const struct
> iovec *iovecs,
>                               unsigned nr_iovecs)
>  {
> @@ -55,6 +72,10 @@ int io_uring_register_files_update(struct io_uring
> *ring, unsigned off,
>         };
>         int ret;
>
> +       ret = bump_rlimit_nofile(nr_files);
> +       if (ret)
> +               return ret;
> +
>
> and it failed with the same as before...
>
> io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7, 8,
> 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> -1, -1, -1, -1,
> -1, ...], 32768) = -1 EMFILE (Too many open files)
>
> if you want i can debug it for you tomorrow? (in london)
>
> >
> > > another thing i think might be a good idea is an io_uring
> > > change/migration log that we update with every kernel release covering
> > > new features but also new restrictions/requirements/tweaks etc.
> >
> > Yes, that is a good idea. The man pages do tend to reference what
> > version included what, but a highlight per release would be a great idea
> > to have without having to dig for it.
> >
> > > something that would take 1 minute to skim and see if relevant.
> > >
> > > because at this point to stay fully updated requires reading all of the
> > > mailing list or checking pulls on your branch + running to binaries
> > > to see if anything breaks.
> >
> > Question is where to post it? Because I would post it here anyway...
>
> i think a txt file in liburing might be the perfect place given the audience
> for it is solely application developers? could start with 5.15 and maintain
> it forward.

actually maybe both places. same text here, but only the incremental.
and the full log on the liburing github page.

>
> >
> > --
> > Jens Axboe
> >

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Jens Axboe]

On 9/18/21 5:19 PM, Victor Stewart wrote:
> On Sat, Sep 18, 2021 at 11:21 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 9/18/21 3:55 PM, Victor Stewart wrote:
>>> On Sat, Sep 18, 2021 at 9:38 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> On Sat, Sep 18, 2021 at 2:26 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>
>>>>> On 9/18/21 2:13 PM, Victor Stewart wrote:
>>>>>> On Sat, Sep 18, 2021 at 3:41 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>
>>>>>>> On 9/18/21 7:41 AM, Victor Stewart wrote:
>>>>>>>> just auto updated from 5.13.16 to 5.13.17, and suddenly my fixed
>>>>>>>> file registrations fail with EOPNOTSUPP using liburing 2.0.
>>>>>>>>
>>>>>>>> static inline struct io_uring ring;
>>>>>>>> static inline int *socketfds;
>>>>>>>>
>>>>>>>> // ...
>>>>>>>>
>>>>>>>> void enableFD(int fd)
>>>>>>>> {
>>>>>>>>    int result = io_uring_register_files_update(&ring, fd,
>>>>>>>>                       &(socketfds[fd] = fd), 1);
>>>>>>>>    printf("enableFD, result = %d\n", result);
>>>>>>>> }
>>>>>>>>
>>>>>>>> maybe this is due to the below and related work that
>>>>>>>> occurred at the end of 5.13 and liburing got out of sync?
>>>>>>>>
>>>>>>>> https://github.com/torvalds/linux/commit/992da01aa932b432ef8dc3885fa76415b5dbe43f#diff-79ffab63f24ef28eec3badbc8769e2a23e0475ab1fbe390207269ece944a0824
>>>>>>>>
>>>>>>>> and can't use liburing 2.1 because of the api changes since 5.13.
>>>>>>>
>>>>>>> That's very strange, the -EOPNOTSUPP should only be possible if you
>>>>>>> are not passing in the ring fd for the register syscall. You should
>>>>>>> be able to mix and match liburing versions just fine, the only exception
>>>>>>> is sometimes between releases (of both liburing and the kernel) where we
>>>>>>> have the liberty to change the API of something that was added before
>>>>>>> release.
>>>>>>>
>>>>>>> Can you do an strace of it and attach?
>>>>>>
>>>>>> oh ya the EOPNOTSUPP was my bug introduced trying to debug.
>>>>>>
>>>>>> here's the real bug...
>>>>>>
>>>>>> io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7,
>>>>>> 8, 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
>>>>>> -1, -1, -1, -1, -1,
>>>>>> -1, ...], 32768) = -1 EMFILE (Too many open files)
>>>>>>
>>>>>> 32,768 is 1U << 15 aka IORING_MAX_FIXED_FILES, but i tried
>>>>>> 16,000 just to try and same issue.
>>>>>>
>>>>>> maybe you're not allowed to have pre-filled (aka non negative 1)
>>>>>> entries upon the initial io_uring_register_files call anymore?
>>>>>>
>>>>>> this was working until the 5.13.16 -> 5.13.17 transition.
>>>>>
>>>>> Ah yes that makes more sense. You need to up RLIMIT_NOFILE, the
>>>>> registered files are under that protection now too. This is also why it
>>>>> was brought back to stable. A bit annoying, but it was needed for the
>>>>> direct file support to have some sanity there.
>>>>>
>>>>> So use rlimit(RLIMIT_NOFILE,...) from the app or ulimit -n to bump the
>>>>> limit.
>>>>
>>>
>>> perfect got it working with..
>>>
>>> struct rlimit maxFilesLimit = {N_IOURING_MAX_FIXED_FILES,
>>> N_IOURING_MAX_FIXED_FILES};
>>> setrlimit(RLIMIT_NOFILE, &maxFilesLimit);
>>
>> Good!
>>
>>>> BTW, this could be incorporated into io_uring_register_files and
>>>> io_uring_register_files_tags(), might not be a bad idea in general. Just
>>>> have it check rlim.rlim_cur for RLIMIT_NOFILE, and if it's smaller than
>>>> 'nr_files', then bump it. That'd hide it nicely, instead of throwing a
>>>> failure.
>>>
>>> the implicit bump sounds like a good idea (at least in theory?).
>>
>> Can you try current liburing -git? Remove your own RLIMIT_NOFILE and
>> just verify that it works. I pushed a change for it.
> 
> i don't have a dev box up right now, but i applied the below changes to 2.0
> sans the tags bit...
> 
> diff --git a/src/register.c b/src/register.c
> index 994aaff..495216a 100644
> --- a/src/register.c
> +++ b/src/register.c
> @@ -7,6 +7,7 @@
>  #include <unistd.h>
>  #include <errno.h>
>  #include <string.h>
> +#include <sys/resource.h>
> 
>  #include "liburing/compat.h"
>  #include "liburing/io_uring.h"
> @@ -14,6 +15,22 @@
> 
>  #include "syscall.h"
> 
> +static int bump_rlimit_nofile(unsigned nr)
> +{
> +       struct rlimit rlim;
> +
> +       if (getrlimit(RLIMIT_NOFILE, &rlim) < 0)
> +               return -errno;
> +       if (rlim.rlim_cur < nr) {
> +               if (nr > rlim.rlim_max)
> +                       return -EMFILE;
> +               rlim.rlim_cur = nr;
> +               setrlimit(RLIMIT_NOFILE, &rlim);
> +       }
> +
> +       return 0;
> +}
> +
>  int io_uring_register_buffers(struct io_uring *ring, const struct
> iovec *iovecs,
>                               unsigned nr_iovecs)
>  {
> @@ -55,6 +72,10 @@ int io_uring_register_files_update(struct io_uring
> *ring, unsigned off,
>         };
>         int ret;
> 
> +       ret = bump_rlimit_nofile(nr_files);
> +       if (ret)
> +               return ret;
> +
> 
> and it failed with the same as before...
> 
> io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7, 8,
> 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> -1, -1, -1, -1,
> -1, ...], 32768) = -1 EMFILE (Too many open files)
> 
> if you want i can debug it for you tomorrow? (in london)

Nah that's fine, I think it's just because you have other files opened
too. We bump the cur limit _to_ 'nr', but that leaves no room for anyone
else. Would be my guess. It works fine for the test case I ran here, but
your case may be different. Does it work if you just make it:

rlim.rlim_cur += nr;

instead?

>>> something that would take 1 minute to skim and see if relevant.
>>>
>>> because at this point to stay fully updated requires reading all of the
>>> mailing list or checking pulls on your branch + running to binaries
>>> to see if anything breaks.
>>
>> Question is where to post it? Because I would post it here anyway...
> 
> i think a txt file in liburing might be the perfect place given the audience
> for it is solely application developers? could start with 5.15 and maintain
> it forward.

Yes, maybe just have a ChangeLog kind of file in there and add a section
for each new release.

-- 
Jens Axboe

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Jens Axboe]

On 9/18/21 5:37 PM, Jens Axboe wrote:
>> and it failed with the same as before...
>>
>> io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7, 8,
>> 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
>> -1, -1, -1, -1,
>> -1, ...], 32768) = -1 EMFILE (Too many open files)
>>
>> if you want i can debug it for you tomorrow? (in london)
> 
> Nah that's fine, I think it's just because you have other files opened
> too. We bump the cur limit _to_ 'nr', but that leaves no room for anyone
> else. Would be my guess. It works fine for the test case I ran here, but
> your case may be different. Does it work if you just make it:
> 
> rlim.rlim_cur += nr;
> 
> instead?

Specifically, just something like the below incremental. If rlim_cur
_seems_ big enough, leave it alone. If not, add the amount we need to
cur. And don't do any error checking here, let's leave failure to the
kernel.

diff --git a/src/register.c b/src/register.c
index bab42d0..7597ec1 100644
--- a/src/register.c
+++ b/src/register.c
@@ -126,9 +126,7 @@ static int bump_rlimit_nofile(unsigned nr)
 	if (getrlimit(RLIMIT_NOFILE, &rlim) < 0)
 		return -errno;
 	if (rlim.rlim_cur < nr) {
-		if (nr > rlim.rlim_max)
-			return -EMFILE;
-		rlim.rlim_cur = nr;
+		rlim.rlim_cur += nr;
 		setrlimit(RLIMIT_NOFILE, &rlim);
 	}
 

-- 
Jens Axboe

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Vito Caputo]

On Sat, Sep 18, 2021 at 05:40:51PM -0600, Jens Axboe wrote:
> On 9/18/21 5:37 PM, Jens Axboe wrote:
> >> and it failed with the same as before...
> >>
> >> io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7, 8,
> >> 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> >> -1, -1, -1, -1,
> >> -1, ...], 32768) = -1 EMFILE (Too many open files)
> >>
> >> if you want i can debug it for you tomorrow? (in london)
> > 
> > Nah that's fine, I think it's just because you have other files opened
> > too. We bump the cur limit _to_ 'nr', but that leaves no room for anyone
> > else. Would be my guess. It works fine for the test case I ran here, but
> > your case may be different. Does it work if you just make it:
> > 
> > rlim.rlim_cur += nr;
> > 
> > instead?
> 
> Specifically, just something like the below incremental. If rlim_cur
> _seems_ big enough, leave it alone. If not, add the amount we need to
> cur. And don't do any error checking here, let's leave failure to the
> kernel.
> 
> diff --git a/src/register.c b/src/register.c
> index bab42d0..7597ec1 100644
> --- a/src/register.c
> +++ b/src/register.c
> @@ -126,9 +126,7 @@ static int bump_rlimit_nofile(unsigned nr)
>  	if (getrlimit(RLIMIT_NOFILE, &rlim) < 0)
>  		return -errno;
>  	if (rlim.rlim_cur < nr) {
> -		if (nr > rlim.rlim_max)
> -			return -EMFILE;
> -		rlim.rlim_cur = nr;
> +		rlim.rlim_cur += nr;
>  		setrlimit(RLIMIT_NOFILE, &rlim);
>  	}
>  
> 

Perhaps it makes more sense to only incur the getrlimit() cost on the
errno=EMFILE path?  As in bump the ulimit and retry the operation on
failure, but when things are OK don't do any of this.

Regards,
Vito Caputo

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Jens Axboe]

On 9/18/21 10:15 PM, Vito Caputo wrote:
> On Sat, Sep 18, 2021 at 05:40:51PM -0600, Jens Axboe wrote:
>> On 9/18/21 5:37 PM, Jens Axboe wrote:
>>>> and it failed with the same as before...
>>>>
>>>> io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7, 8,
>>>> 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
>>>> -1, -1, -1, -1,
>>>> -1, ...], 32768) = -1 EMFILE (Too many open files)
>>>>
>>>> if you want i can debug it for you tomorrow? (in london)
>>>
>>> Nah that's fine, I think it's just because you have other files opened
>>> too. We bump the cur limit _to_ 'nr', but that leaves no room for anyone
>>> else. Would be my guess. It works fine for the test case I ran here, but
>>> your case may be different. Does it work if you just make it:
>>>
>>> rlim.rlim_cur += nr;
>>>
>>> instead?
>>
>> Specifically, just something like the below incremental. If rlim_cur
>> _seems_ big enough, leave it alone. If not, add the amount we need to
>> cur. And don't do any error checking here, let's leave failure to the
>> kernel.
>>
>> diff --git a/src/register.c b/src/register.c
>> index bab42d0..7597ec1 100644
>> --- a/src/register.c
>> +++ b/src/register.c
>> @@ -126,9 +126,7 @@ static int bump_rlimit_nofile(unsigned nr)
>>  	if (getrlimit(RLIMIT_NOFILE, &rlim) < 0)
>>  		return -errno;
>>  	if (rlim.rlim_cur < nr) {
>> -		if (nr > rlim.rlim_max)
>> -			return -EMFILE;
>> -		rlim.rlim_cur = nr;
>> +		rlim.rlim_cur += nr;
>>  		setrlimit(RLIMIT_NOFILE, &rlim);
>>  	}
>>  
>>
> 
> Perhaps it makes more sense to only incur the getrlimit() cost on the
> errno=EMFILE path?  As in bump the ulimit and retry the operation on
> failure, but when things are OK don't do any of this.

Yes, may as well. I've pushed a change that makes it incremental and
doesn't trigger it unless we hit EMFILE.

-- 
Jens Axboe

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Victor Stewart]

On Sun, Sep 19, 2021 at 12:40 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 9/18/21 5:37 PM, Jens Axboe wrote:
> >> and it failed with the same as before...
> >>
> >> io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7, 8,
> >> 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> >> -1, -1, -1, -1,
> >> -1, ...], 32768) = -1 EMFILE (Too many open files)
> >>
> >> if you want i can debug it for you tomorrow? (in london)
> >
> > Nah that's fine, I think it's just because you have other files opened
> > too. We bump the cur limit _to_ 'nr', but that leaves no room for anyone
> > else. Would be my guess. It works fine for the test case I ran here, but
> > your case may be different. Does it work if you just make it:
> >
> > rlim.rlim_cur += nr;
> >
> > instead?
>
> Specifically, just something like the below incremental. If rlim_cur
> _seems_ big enough, leave it alone. If not, add the amount we need to
> cur. And don't do any error checking here, let's leave failure to the
> kernel.
>
> diff --git a/src/register.c b/src/register.c
> index bab42d0..7597ec1 100644
> --- a/src/register.c
> +++ b/src/register.c
> @@ -126,9 +126,7 @@ static int bump_rlimit_nofile(unsigned nr)
>         if (getrlimit(RLIMIT_NOFILE, &rlim) < 0)
>                 return -errno;
>         if (rlim.rlim_cur < nr) {
> -               if (nr > rlim.rlim_max)
> -                       return -EMFILE;
> -               rlim.rlim_cur = nr;
> +               rlim.rlim_cur += nr;
>                 setrlimit(RLIMIT_NOFILE, &rlim);
>         }
>

i just tried this patch and same failure. if you intend for this to remain
in the code i can debug what's going on?

>
> --
> Jens Axboe
>

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Jens Axboe]

On Sep 20, 2021, at 6:51 AM, Victor Stewart <v@nametag.social> wrote:
> 
> On Sun, Sep 19, 2021 at 12:40 AM Jens Axboe <axboe@kernel.dk> wrote:
>> 
>> On 9/18/21 5:37 PM, Jens Axboe wrote:
>>>> and it failed with the same as before...
>>>> 
>>>> io_uring_register(13, IORING_REGISTER_FILES, [-1, -1, -1, 3, 4, 5, 6, 7, 8,
>>>> 9, 10, 11, 12, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
>>>> -1, -1, -1, -1,
>>>> -1, ...], 32768) = -1 EMFILE (Too many open files)
>>>> 
>>>> if you want i can debug it for you tomorrow? (in london)
>>> 
>>> Nah that's fine, I think it's just because you have other files opened
>>> too. We bump the cur limit _to_ 'nr', but that leaves no room for anyone
>>> else. Would be my guess. It works fine for the test case I ran here, but
>>> your case may be different. Does it work if you just make it:
>>> 
>>> rlim.rlim_cur += nr;
>>> 
>>> instead?
>> 
>> Specifically, just something like the below incremental. If rlim_cur
>> _seems_ big enough, leave it alone. If not, add the amount we need to
>> cur. And don't do any error checking here, let's leave failure to the
>> kernel.
>> 
>> diff --git a/src/register.c b/src/register.c
>> index bab42d0..7597ec1 100644
>> --- a/src/register.c
>> +++ b/src/register.c
>> @@ -126,9 +126,7 @@ static int bump_rlimit_nofile(unsigned nr)
>>        if (getrlimit(RLIMIT_NOFILE, &rlim) < 0)
>>                return -errno;
>>        if (rlim.rlim_cur < nr) {
>> -               if (nr > rlim.rlim_max)
>> -                       return -EMFILE;
>> -               rlim.rlim_cur = nr;
>> +               rlim.rlim_cur += nr;
>>                setrlimit(RLIMIT_NOFILE, &rlim);
>>        }
>> 
> 
> i just tried this patch and same failure. if you intend for this to remain
> in the code i can debug what's going on?

Yes please do, thanks. 

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Victor Stewart]

> > i just tried this patch and same failure. if you intend for this to remain
> > in the code i can debug what's going on?
>
> Yes please do, thanks.

scratch that works great.

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Pavel Begunkov]

On 9/18/21 11:21 PM, Jens Axboe wrote:
> On 9/18/21 3:55 PM, Victor Stewart wrote:
>>> BTW, this could be incorporated into io_uring_register_files and
>>> io_uring_register_files_tags(), might not be a bad idea in general. Just
>>> have it check rlim.rlim_cur for RLIMIT_NOFILE, and if it's smaller than
>>> 'nr_files', then bump it. That'd hide it nicely, instead of throwing a
>>> failure.
>>
>> the implicit bump sounds like a good idea (at least in theory?).
> 
> Can you try current liburing -git? Remove your own RLIMIT_NOFILE and
> just verify that it works. I pushed a change for it.

Sounds like it pretty easy can be a very unexpected behaviour. Do many
libraries / etc. implicitly tinker with it?

>> another thing i think might be a good idea is an io_uring
>> change/migration log that we update with every kernel release covering
>> new features but also new restrictions/requirements/tweaks etc.
> 
> Yes, that is a good idea. The man pages do tend to reference what
> version included what, but a highlight per release would be a great idea
> to have without having to dig for it.
> 
>> something that would take 1 minute to skim and see if relevant.
>>
>> because at this point to stay fully updated requires reading all of the
>> mailing list or checking pulls on your branch + running to binaries
>> to see if anything breaks.
> 
> Question is where to post it? Because I would post it here anyway...

Good idea. We need it in a single file to be useful

liburing/changelog.txt?

-- 
Pavel Begunkov

Re: [BUG? liburing] io_uring_register_files_update with liburing 2.0 on 5.13.17

[Jens Axboe]

On 9/19/21 5:56 AM, Pavel Begunkov wrote:
> On 9/18/21 11:21 PM, Jens Axboe wrote:
>> On 9/18/21 3:55 PM, Victor Stewart wrote:
>>>> BTW, this could be incorporated into io_uring_register_files and
>>>> io_uring_register_files_tags(), might not be a bad idea in general. Just
>>>> have it check rlim.rlim_cur for RLIMIT_NOFILE, and if it's smaller than
>>>> 'nr_files', then bump it. That'd hide it nicely, instead of throwing a
>>>> failure.
>>>
>>> the implicit bump sounds like a good idea (at least in theory?).
>>
>> Can you try current liburing -git? Remove your own RLIMIT_NOFILE and
>> just verify that it works. I pushed a change for it.
> 
> Sounds like it pretty easy can be a very unexpected behaviour. Do many
> libraries / etc. implicitly tinker with it?

Don't know if they do, but as long as we simply increase probably not a
huge problem, even if not the prettiest. I just wish we had done this
for the 2.1 release, to avoid people running into this for stable
backports. At least it's a fairly well known (and specific) error code
for this case.

-- 
Jens Axboe