Re: [RFC PATCH 1/2] mm: swap: check if swap backing device is congested or not

[Tim Chen <tim.c.chen@linux.intel.com>]

On 12/19/18 10:40 AM, Yang Shi wrote:
> 
> 

>>>> I don't think your dereference inode = si->swap_file->f_mapping->host
>>>> is always safe.  You should do it only when (si->flags & SWP_FS) is true.
>>> Do you mean it is not safe for swap partition?
>> The f_mapping may not be instantiated.  It is only done for SWP_FS.
> 
> Really? I saw the below calls in swapon:
> 
> swap_file = file_open_name(name, O_RDWR|O_LARGEFILE, 0);
> ...
> p->swap_file = swap_file;
> mapping = swap_file->f_mapping;
> inode = mapping->host;
> ...
> 
> Then the below code manipulates the inode.
> 
> And, trace shows file_open_name() does call blkdev_open if it is turning block device swap on. And, blkdev_open() would return instantiated address_space and inode.
> 
> Am I missing something?
> 

I was trying to limit the congestion logic for block devices backed swap.
So the check I had in mind should really be "si->flags & SWP_BLKDEV"
instead of si->flags & SWP_FS.  I was concerned that there could
be other use cases where the inode dereference is invalid.

Looking at the code a bit more, looks like swap_cluster_readahead is not
used for other special case swap usage (like page migration).  So
you would a proper swapfile and inode here.  But I think it is still
a good idea to have a check for SWP_BLKDEV in si->flags.

Thanks.

Tim