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

[Jens Axboe <axboe@kernel.dk>]

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