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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 1/29/19 4:44 PM, Jens Axboe wrote:
> On 1/29/19 4:31 PM, Jann Horn wrote:
>> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@xxxxxxxxx> 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.

Fixed it!

-- 
Jens Axboe




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux