On 12/16/18 9:37 AM, Christoph Hellwig wrote:
> [adding Al, I think we really want him to review anything fget
> related]
>
> On Thu, Dec 13, 2018 at 10:56:35AM -0700, Jens Axboe wrote:
>> Some uses cases repeatedly get and put references to the same file, but
>> the only exposed interface is doing these one at the time. As each of
>> these entail an atomic inc or dec on a shared structure, that cost can
>> add up.
>>
>> Add fget_many(), which works just like fget(), except it takes an
>> argument for how many references to get on the file. Ditto fput_many(),
>> which can drop an arbitrary number of references to a file.
>>
>> Signed-off-by: Jens Axboe <[email protected]>
>> ---
>> fs/file.c | 15 ++++++++++-----
>> fs/file_table.c | 10 ++++++++--
>> include/linux/file.h | 2 ++
>> include/linux/fs.h | 3 ++-
>> 4 files changed, 22 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/file.c b/fs/file.c
>> index 7ffd6e9d103d..ad9870edfd51 100644
>> --- a/fs/file.c
>> +++ b/fs/file.c
>> @@ -676,7 +676,7 @@ void do_close_on_exec(struct files_struct *files)
>> spin_unlock(&files->file_lock);
>> }
>>
>> -static struct file *__fget(unsigned int fd, fmode_t mask)
>> +static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
>> {
>> struct files_struct *files = current->files;
>> struct file *file;
>> @@ -691,7 +691,7 @@ static struct file *__fget(unsigned int fd, fmode_t mask)
>> */
>> if (file->f_mode & mask)
>> file = NULL;
>> - else if (!get_file_rcu(file))
>> + else if (!get_file_rcu_many(file, refs))
>> goto loop;
>> }
>> rcu_read_unlock();
>> @@ -699,15 +699,20 @@ static struct file *__fget(unsigned int fd, fmode_t
>> mask)
>> return file;
>> }
>>
>> +struct file *fget_many(unsigned int fd, unsigned int refs)
>> +{
>> + return __fget(fd, FMODE_PATH, refs);
>> +}
>> +
>> struct file *fget(unsigned int fd)
>> {
>> - return __fget(fd, FMODE_PATH);
>> + return fget_many(fd, 1);
>
> Can we just call __fget directly here? That is a little easier
> to follow, and might actually generate better code if the compiler
> is inliner challenged (which they often seem to be).
Certainly, it'd be the same thing. I kind of like having it follow the
natural path to get there, unless there are strong objections to
bypassing that step.
>> +void fput(struct file *file)
>> +{
>> + fput_many(file, 1);
>> +}
>> +
>> +
>
> Double empty line here.
Fixed.
>> +#define get_file_rcu_many(x, cnt) atomic_long_add_unless(&(x)->f_count,
>> (cnt), 0)
>
> This could use a line break to be easier readable and not spill over
> 80 chars. Otherwise this looks fine to me.
Fixed.
--
Jens Axboe
