[RFC] what the hell is ->f_mapping->host->i_mapping thing about?

[RFC] what the hell is ->f_mapping->host->i_mapping thing about?

[Al Viro]

	In do_dentry_open() we have the following weirdness:

        file_ra_state_init(&f->f_ra, f->f_mapping->host->i_mapping);

What is it about?  How and when can ->f_mapping->host->i_mapping be *NOT*
equal to ->f_mapping?

It came from
commit 1c211088833a27daa4512348bcae9890e8cf92d4
Author: Andrew Morton <akpm@osdl.org>
Date:   Wed May 26 17:35:42 2004 -0700

    [PATCH] Fix the setting of file->f_ra on block-special files
	
    We need to set file->f_ra _after_ calling blkdev_open(), when inode->i_mapping
    points at the right thing.  And we need to get it from
    inode->i_mapping->host->i_mapping too, which represents the underlying device.

    Also, don't test for null file->f_mapping in the O_DIRECT checks.

Sure, we need to set ->f_ra after ->open(), since ->f_mapping might be
changed by ->open().  No arguments here - that call should've been moved.
But what the hell has the last bit come from?  What am I missing here?
IDGI...

And that gift keeps giving -
fs/nfs/nfs4file.c:388:  file_ra_state_init(&filep->f_ra, filep->f_mapping->host->i_mapping);
is a copy of that thing.  Equally bogus, AFAICT...

Re: [RFC] what the hell is ->f_mapping->host->i_mapping thing about?

[Eric Biggers]

On Sun, Jun 20, 2021 at 10:42:57PM +0000, Al Viro wrote:
> 	In do_dentry_open() we have the following weirdness:
> 
>         file_ra_state_init(&f->f_ra, f->f_mapping->host->i_mapping);
> 
> What is it about?  How and when can ->f_mapping->host->i_mapping be *NOT*
> equal to ->f_mapping?
> 
> It came from
> commit 1c211088833a27daa4512348bcae9890e8cf92d4
> Author: Andrew Morton <akpm@osdl.org>
> Date:   Wed May 26 17:35:42 2004 -0700
> 
>     [PATCH] Fix the setting of file->f_ra on block-special files
> 	
>     We need to set file->f_ra _after_ calling blkdev_open(), when inode->i_mapping
>     points at the right thing.  And we need to get it from
>     inode->i_mapping->host->i_mapping too, which represents the underlying device.
> 
>     Also, don't test for null file->f_mapping in the O_DIRECT checks.
> 
> Sure, we need to set ->f_ra after ->open(), since ->f_mapping might be
> changed by ->open().  No arguments here - that call should've been moved.
> But what the hell has the last bit come from?  What am I missing here?
> IDGI...
> 
> And that gift keeps giving -
> fs/nfs/nfs4file.c:388:  file_ra_state_init(&filep->f_ra, filep->f_mapping->host->i_mapping);
> is a copy of that thing.  Equally bogus, AFAICT...

FWIW, I came to the same conclusion that just ->f_mapping would be sufficient:
https://lkml.kernel.org/linux-fsdevel/20170326032128.32721-1-ebiggers3@gmail.com

- Eric

Re: [RFC] what the hell is ->f_mapping->host->i_mapping thing about?

[Al Viro]

On Mon, Jun 21, 2021 at 10:45:33AM -0700, Eric Biggers wrote:
> On Sun, Jun 20, 2021 at 10:42:57PM +0000, Al Viro wrote:
> > 	In do_dentry_open() we have the following weirdness:
> > 
> >         file_ra_state_init(&f->f_ra, f->f_mapping->host->i_mapping);
> > 
> > What is it about?  How and when can ->f_mapping->host->i_mapping be *NOT*
> > equal to ->f_mapping?
> > 
> > It came from
> > commit 1c211088833a27daa4512348bcae9890e8cf92d4
> > Author: Andrew Morton <akpm@osdl.org>
> > Date:   Wed May 26 17:35:42 2004 -0700
> > 
> >     [PATCH] Fix the setting of file->f_ra on block-special files
> > 	
> >     We need to set file->f_ra _after_ calling blkdev_open(), when inode->i_mapping
> >     points at the right thing.  And we need to get it from
> >     inode->i_mapping->host->i_mapping too, which represents the underlying device.
> > 
> >     Also, don't test for null file->f_mapping in the O_DIRECT checks.
> > 
> > Sure, we need to set ->f_ra after ->open(), since ->f_mapping might be
> > changed by ->open().  No arguments here - that call should've been moved.
> > But what the hell has the last bit come from?  What am I missing here?
> > IDGI...
> > 
> > And that gift keeps giving -
> > fs/nfs/nfs4file.c:388:  file_ra_state_init(&filep->f_ra, filep->f_mapping->host->i_mapping);
> > is a copy of that thing.  Equally bogus, AFAICT...
> 
> FWIW, I came to the same conclusion that just ->f_mapping would be sufficient:
> https://lkml.kernel.org/linux-fsdevel/20170326032128.32721-1-ebiggers3@gmail.com

Sorry, missed it back then ;-/