On 12/19/18 3:48 PM, Yang Shi wrote:
>
>
> On 12/19/18 11:00 AM, Tim Chen wrote:
>> 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
>
> Yes, just swap page fault and shmem calls this function. Actually, your above
> concern is valid if the inode were added into swap_address_space
> (address_space->host). I did this in my first attempt, and found out it may
> break some assumption in clear_page_dirty_for_io() and migration.
>
> So, I made the patch as it is.
>
>> a good idea to have a check for SWP_BLKDEV in si->flags.
>
> I don't get your point why it should be block dev swap only. IMHO, block dev
> swap should be less likely fragmented and congested than swap file, right?
> Block dev swap could be a dedicated physical device, but swap file has to be
> with filesystem.
>
> It sounds reasonable to me to have this check for swap file only. However, to
> be honest, it sounds not hurt to have both.
>
Yes, I think we want to do it for both cases.
My original concern was that the backing store was not sitting on
a block device for some special case swap usage. And
si->swap_file->f_mapping->host
may fails to dereference to a host inode that's a valid block device.
It looks like on the call paths si->flags should either be SWP_BLKDEV or SWP_FS
on all the call paths. So si->swap_file->f_mapping->host should be valid
and your code is safe.
If we want to be paranoid we may do
if (si->flags & (SWP_BLKDEV | SWP_FS)) {
if (inode_read_congested(si->swap_file->f_mapping->host))
goto skip;
}
Thanks.
Tim
