Re: [PATCH 09/18] io_uring: use fget/fput_many() for file references

[Jens Axboe <axboe@kernel.dk>]

On 1/29/19 4:31 PM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>> Add a separate io_submit_state structure, to cache some of the things
>> we need for IO submission.
>>
>> One such example is file reference batching. io_submit_state. We get as
>> many references as the number of sqes we are submitting, and drop
>> unused ones if we end up switching files. The assumption here is that
>> we're usually only dealing with one fd, and if there are multiple,
>> hopefuly they are at least somewhat ordered. Could trivially be extended
>> to cover multiple fds, if needed.
>>
>> On the completion side we do the same thing, except this is trivially
>> done just locally in io_iopoll_reap().
> [...]
>> +static void io_file_put(struct io_submit_state *state, struct file *file)
>> +{
>> +       if (!state) {
>> +               fput(file);
>> +       } else if (state->file) {
>> +               int diff = state->has_refs - state->used_refs;
>> +
>> +               if (diff)
>> +                       fput_many(state->file, diff);
>> +               state->file = NULL;
>> +       }
>> +}
> 
> Hmm, this function confuses me.
> The state==NULL path works as I'd expect, it calls fput() on the file.
> But if `state!=NULL && state->file==NULL`, it does nothing, it never
> uses `file`.
> And if `state->file!=NULL`, it drops the excess bias on the file's
> refcount, but it doesn't drop the current reference - and again
> without even looking at `file`.
> 
> So when io_prep_rw() uses io_file_get() to grab a reference on a file
> it hasn't seen before, it will acquire `ios_left` references and
> actually use one of them; then if it goes through the out_fput error
> path, it goes through the path for `state->file!=NULL`, drops
> `ios_left-1` references (leaving the refcount elevated by 1), and
> forgets about the file?

I'll take a look, it's not impossible there's an off-by-one there.

>> +/*
>> + * Get as many references to a file as we have IOs left in this submission,
>> + * assuming most submissions are for one file, or at least that each file
>> + * has more than one submission.
>> + */
>> +static struct file *io_file_get(struct io_submit_state *state, int fd)
>> +{
>> +       if (!state)
>> +               return fget(fd);
>> +
>> +       if (state->file) {
>> +               if (state->fd == fd) {
>> +                       state->used_refs++;
>> +                       state->ios_left--;
>> +                       return state->file;
>> +               }
>> +               io_file_put(state, NULL);
>> +       }
>> +       state->file = fget_many(fd, state->ios_left);
>> +       if (!state->file)
>> +               return NULL;
>> +
>> +       state->fd = fd;
>> +       state->has_refs = state->ios_left;
>> +       state->used_refs = 1;
>> +       state->ios_left--;
>> +       return state->file;
>> +}
>> +
>>  static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>> -                     bool force_nonblock)
>> +                     bool force_nonblock, struct io_submit_state *state)
>>  {
>>         struct io_ring_ctx *ctx = req->ctx;
>>         struct kiocb *kiocb = &req->rw;
>> @@ -487,7 +560,7 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>>         int fd, ret;
>>
>>         fd = READ_ONCE(sqe->fd);
>> -       kiocb->ki_filp = fget(fd);
>> +       kiocb->ki_filp = io_file_get(state, fd);
>>         if (unlikely(!kiocb->ki_filp))
>>                 return -EBADF;
>>         kiocb->ki_pos = READ_ONCE(sqe->off);
>> @@ -528,7 +601,7 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>>         }
>>         return 0;
>>  out_fput:
>> -       fput(kiocb->ki_filp);
>> +       io_file_put(state, kiocb->ki_filp);
>>         return ret;
>>  }
> [...]
>> +static void io_submit_state_start(struct io_submit_state *state,
>> +                                 struct io_ring_ctx *ctx, unsigned max_ios)
> 
> There are various places in your series where you use raw "unsigned"
> instead of "unsigned int"; when I run your tree through checkpatch.pl,
> it complains about that and a few other things. Please fix the
> checkpatch warnings (except for warnings where you know that they
> shouldn't apply here for some reason).

Using unsigned is just fine, it's the same thing. checkpatch.pl
complains about a lot of stuff that doesn't matter, that's one of them.
I don't mind fixing valid warnings, but this particular one is just
noise.

-- 
Jens Axboe