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

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

[Dan Carpenter]

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)

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

  4201                  goto out;
  4202  
  4203          /* Prevent new folios from being added to the inode. */
  4204          filemap_invalidate_lock(mapping);
  4205  
  4206          if (!mapping->nrpages)
  4207                  goto unlock;
  4208  
  4209          unmap_mapping_pages(mapping, first, nr, false);
  4210  
  4211          /* Write back the data if we're asked to. */
  4212          if (flush) {
  4213                  struct writeback_control wbc = {
  4214                          .sync_mode      = WB_SYNC_ALL,
  4215                          .nr_to_write    = LONG_MAX,
  4216                          .range_start    = start,
  4217                          .range_end      = end,
  4218                  };
  4219  
  4220                  filemap_fdatawrite_wbc(mapping, &wbc);
  4221          }
  4222  
  4223          /* Wait for writeback to complete on all folios and discard. */
  4224          truncate_inode_pages_range(mapping, start, end);
  4225  
  4226  unlock:
  4227          filemap_invalidate_unlock(mapping);
  4228  out:
  4229          return filemap_check_errors(mapping);
                                            ^^^^^^^
Then we're toasted.  The errors from filemap_check_errors() aren't what
I expected.

  4230  }

regards,
dan carpenter

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.