On Thu 22-04-21 11:57:14, Bob Peterson wrote:
> ----- Original Message -----
> > OK, thanks for explanation! I missed that GFS2 glocks are task local. But
> > then I have another question. We have the following:
> >
> > gfs2_file_read_iter()
> > grabs inode glock in shared mode
> > generic_file_read_iter()
> > filemap_read()
> > copy_page_to_iter()
> > <page fault>
> > grabs mmap_sem
> > gfs2_fault()
> > - tries to grab inode glock again -> possible recursive locking
> >
> > And even if the locking isn't recursive, you have glock->mmap_sem and
> > mmap_sem->glock lock orderings so ABBA deadlocks are possible AFAICT.
> >
> > And there's a similar problem with the write path as well, just the lock is
> > grabbed exclusively.
> >
> > Honza
> > --
> > Jan Kara <[email protected]>
> > SUSE Labs, CR
>
> Hi Jan,
>
> IIRC, gfs2 tries to prevent the page fault by having gfs2_file_read_iter
> call into generic_file_read_iter twice:
>
> The first time without the glock held, but specifying IOCB_NOIO, which
> brings the pages in and returns -EAGAIN. Then it acquires the glock,
> then calls into generic_file_read_iter a second time.
>
> I'm not sure if that prevents the deadlock or just makes it less likely.
> I still need to investigate that.
Aha, I missed that trick. Thanks for pointing that out. But indeed this
just makes the race harder to hit. If already the first page is not in the
page cache and needs to be fetched from the disk, nothing is faulted in and
you have to do everything in the call with glock held. And even if you
prefaulted the whole buffer, there's no guarantee it doesn't get reclaimed
again before you reach copy_page_to_iter().
Also the write path doesn't seem to play any tricks like this and is prone
to the same kind of problem:
iomap_file_buffered_write()
iomap_apply()
gfs2_iomap_begin() - takes glock
iomap_write_actor()
iov_iter_fault_in_readable()
<fault>
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
