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
