Re: [PATCH] xfs: use MMAPLOCK around filemap_map_pages()

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Dave Chinner <david@fromorbit.com>,
	linux-xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH] xfs: use MMAPLOCK around filemap_map_pages()
Date: Tue, 30 Jun 2020 11:26:45 -0700	[thread overview]
Message-ID: <20200630182645.GQ7606@magnolia> (raw)
In-Reply-To: <CAOQ4uxiuEVW=d+g_3kj+zdTc_ngEkF+nGnJ+M2g1aU3SqsFa+w@mail.gmail.com>

On Tue, Jun 30, 2020 at 06:23:12PM +0300, Amir Goldstein wrote:
> > /me wonders if someone could please check all the *_ops that point to
> > generic helpers to see if we're missing obvious things like lock
> > taking.  Particularly someone who wants to learn about xfs' locking
> > strategy; I promise it won't let out a ton of bees.
> >
> 
> The list was compiled manually by auditing 'git grep '_operations.*=' fs/xfs'
> structs for non xfs_/iomap_/noop_ functions.
> I am not sure if all iomap_ functions are safe in that respect, but I suppose
> those were done recently with sufficient xfs developers review...

The iomap functions shouldn't be taking/releasing any locks at all; it's
up to the filesystem to provide the concurrency controls.

> fs/xfs/xfs_aops.c:const struct address_space_operations
> xfs_address_space_operations = {
>         .error_remove_page      = generic_error_remove_page,
> 
> generic_error_remove_page() calls truncate_inode_page() without MMAPLOCK
> Is that safe? not sure

/me has a funny feeling it isn't, since this does the same thing to the
pagecache as a holepunch.

> fs/xfs/xfs_file.c:static const struct vm_operations_struct xfs_file_vm_ops = {
>         .map_pages      = filemap_map_pages,
> 
> Fixed by $SUBJECT
> 
> fs/xfs/xfs_file.c:const struct file_operations xfs_file_operations = {
>         .splice_read    = generic_file_splice_read,
> 
> Will call xfs_file_read_iter, so looks fine
> 
>        .splice_write   = iter_file_splice_write,
> 
> Will call xfs_file_write_iter, so looks fine
> 
>        .get_unmapped_area = thp_get_unmapped_area,
> 
> Looks fine?
> 
> fs/xfs/xfs_file.c:const struct file_operations xfs_dir_file_operations = {
>         .read           = generic_read_dir,
>         .llseek         = generic_file_llseek,
> 
> No page cache, no dio, no worries?

Right.

--D

> Thanks,
> Amir.