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.
