Re: [bug report] mm: Provide a means of invalidation without using launder_folio

Re: [bug report] mm: Provide a means of invalidation without using launder_folio

[Matthew Wilcox]

On Thu, Jun 13, 2024 at 04:55:30PM +0300, Dan Carpenter wrote:
> Hello David Howells,
> 
> Commit 74e797d79cf1 ("mm: Provide a means of invalidation without
> using launder_folio") from Mar 27, 2024 (linux-next), leads to the
> following Smatch static checker warning:
> 
> 	mm/filemap.c:4229 filemap_invalidate_inode()
> 	error: we previously assumed 'mapping' could be null (see line 4200)

I think David has been overly cautious here.  I don't think i_mapping
can ever be NULL.  inode_init_always() sets i_mapping to be
&inode->i_data and I don't see anywhere that changes i_mapping to be
NULL.

> mm/filemap.c
>   4192  int filemap_invalidate_inode(struct inode *inode, bool flush,
>   4193                               loff_t start, loff_t end)
>   4194  {
>   4195          struct address_space *mapping = inode->i_mapping;
>   4196          pgoff_t first = start >> PAGE_SHIFT;
>   4197          pgoff_t last = end >> PAGE_SHIFT;
>   4198          pgoff_t nr = end == LLONG_MAX ? ULONG_MAX : last - first + 1;
>   4199  
>   4200          if (!mapping || !mapping->nrpages || end < start)
>                     ^^^^^^^^
> If mapping is NULL

Re: [bug report] mm: Provide a means of invalidation without using launder_folio

[Dan Carpenter]

On Thu, Jun 13, 2024 at 03:56:23PM +0100, Matthew Wilcox wrote:
> On Thu, Jun 13, 2024 at 04:55:30PM +0300, Dan Carpenter wrote:
> > Hello David Howells,
> > 
> > Commit 74e797d79cf1 ("mm: Provide a means of invalidation without
> > using launder_folio") from Mar 27, 2024 (linux-next), leads to the
> > following Smatch static checker warning:
> > 
> > 	mm/filemap.c:4229 filemap_invalidate_inode()
> > 	error: we previously assumed 'mapping' could be null (see line 4200)
> 
> I think David has been overly cautious here.  I don't think i_mapping
> can ever be NULL.  inode_init_always() sets i_mapping to be
> &inode->i_data and I don't see anywhere that changes i_mapping to be
> NULL.
> 

I don't really understand the errors from this function, though...  I
would have expected it to return -EINVAL on this path but it instead
looks up if any error flags as set in the mapping, otherwise it returns
success.

regards,
dan carpenter

Re: [bug report] mm: Provide a means of invalidation without using launder_folio

[Matthew Wilcox]

On Thu, Jun 13, 2024 at 06:13:19PM +0300, Dan Carpenter wrote:
> On Thu, Jun 13, 2024 at 03:56:23PM +0100, Matthew Wilcox wrote:
> > On Thu, Jun 13, 2024 at 04:55:30PM +0300, Dan Carpenter wrote:
> > > Hello David Howells,
> > > 
> > > Commit 74e797d79cf1 ("mm: Provide a means of invalidation without
> > > using launder_folio") from Mar 27, 2024 (linux-next), leads to the
> > > following Smatch static checker warning:
> > > 
> > > 	mm/filemap.c:4229 filemap_invalidate_inode()
> > > 	error: we previously assumed 'mapping' could be null (see line 4200)
> > 
> > I think David has been overly cautious here.  I don't think i_mapping
> > can ever be NULL.  inode_init_always() sets i_mapping to be
> > &inode->i_data and I don't see anywhere that changes i_mapping to be
> > NULL.
> > 
> 
> I don't really understand the errors from this function, though...  I
> would have expected it to return -EINVAL on this path but it instead
> looks up if any error flags as set in the mapping, otherwise it returns
> success.

Assuming that the '!mapping' check is just a thinko, then it makes
sense.  The other two things being tested are (a) Are there no pages
in the mapping? (b) Is the end of the range before the start.

In either case, we have invalidated "all" of the pages in the range
(since there are no pages in the range), so we want to return success.