On Tue, 27 Jul 2021, Goldwyn Rodrigues wrote: > On 12:25 27/07, NeilBrown wrote: > > On Tue, 27 Jul 2021, Matthew Wilcox wrote: > > > On Tue, Jul 27, 2021 at 08:06:21AM +1000, NeilBrown wrote: > > > > You seem to be assuming that inode->i_mapping->host is always 'inode'. > > > > That is not the case. > > > > > > Weeeelllll ... technically, outside of the filesystems that are > > > changed here, the only assumption in common code that is made is that > > > inode_to_bdi(inode->i_mapping->host->i_mapping->host) == > > > inode_to_bdi(inode) > > > > Individual filesystems doing their own thing is fine. Passing just an > > inode to inode_to_bdi is fine. > > > > But the patch changes do_dentry_open() > > But do_dentry_open() is setting up the file pointer (f) based on > inode (and it's i_mapping). Can f->f_mapping change within > do_dentry_open()? do_dentry_open calls file_ra_state_init() to copy ra_pages from the bdi for inode->i_mapping->host->i_mapping. I do think there is some pointless indirection here, and it should be sufficient to pass inode->i_mapping (aka f->f_mapping) to file_ra_state_init(). (though in 2004, Andrew Morton thought otherwise) But you have changed do_dentry_open() to not follow the ->i_mapping link at all. So in the coda case f->f_ra will be inititalied from the bdi for coda instead of the bdi for the filesystem coda uses for local storage. So this is a change in behaviour. Maybe not a serious one, but one that needs to be understood. https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=1c211088833a27daa4512348bcae9890e8cf92d4 Hmm. drivers/dax/device.c does some funky things with ->i_mapping too. I wonder if that would be affected by this change.... probably not, it looks like it is the same super_block and so the same ra info for both mappings. NeilBrown > > > > > > > > > Looking at inode_to_bdi, that just means that they have the same i_sb. > > > Which is ... not true for character raw devices? > > > if (++raw_devices[minor].inuse == 1) > > > file_inode(filp)->i_mapping = > > > bdev->bd_inode->i_mapping; > > > but then, who's using readahead on a character raw device? They > > > force O_DIRECT. But maybe this should pass inode->i_mapping->host > > > instead of inode. > > > > Also not true in coda. > > > > coda (for those who don't know) is a network filesystem which fetches > > whole files (and often multiple files) at a time (like the Andrew > > filesystem). The files are stored in a local filesystem which acts as a > > cache. > > > > So an inode in a 'coda' filesystem access page-cache pages from a file > > in e.g. an 'ext4' filesystem. This is done via the ->i_mapping link. > > For (nearly?) all other filesystems, ->i_mapping is a link to ->i_data > > in the same inode. > > > > > > > > > In particular, fs/coda/file.c contains > > > > > > > > if (coda_inode->i_mapping == &coda_inode->i_data) > > > > coda_inode->i_mapping = host_inode->i_mapping; > > > > > > > > So a "coda_inode" shares the mapping with a "host_inode". > > > > > > > > This is why an inode has both i_data and i_mapping. > > > > > > > > So I'm not really sure this patch is safe. It might break codafs. > > > > > > > > But it is more likely that codafs isn't used, doesn't work, should be > > > > removed, and i_data should be renamed to i_mapping. > > > > > > I think there's also something unusual going on with either ocfs2 > > > or gfs2. But yes, I don't understand the rules for when I need to > > > go from inode->i_mapping->host. > > > > > > > Simple. Whenever you want to work with the page-cache pages, you cannot > > assume anything in the original inode is relevant except i_mapping (and > > maybe i_size I guess). > > > > NeilBrown > > -- > Goldwyn > >
