Re: [PATCH RFC 2/2] btrfs: space-info: Don't allow signal to interrupt ticket waiting

David Sterba
2020-07-07T16:17:15Z

On Mon, Jul 06, 2020 at 10:33:56AM -0400, Josef Bacik wrote:
> On 7/6/20 10:05 AM, Qu Wenruo wrote:
> >> This only needs to be handled for FLUSH_ALL and FLUSH_STEAL.  Anybody
> >> doing btrfs_start_transaction() should be able to fail with -EINTR,
> >> because they should be close to the syscall path.  Balance needs to be
> >> fixed to deal with it, and I assume there might be a few other places,
> >> but by-in-large none of these places should flip read only.  Thanks,
> >
> > There are already ones existing, for btrfs_drop_snapshot().
> >
> > This is mostly caused by btrfs_delalloc_reserve_metadata(), which always
> > use FLUSH_ALL unless there is a running trans or its space cache inode.
> >
> > But still, for a lot of relocation code, we don't really want to bother
> > the EINTR and just want uninterruptible at least for now.
> >
> > Any idea for that?
> > Or just rework how we handle errors in a lot of places?
> >
> > It doesn't look correct for such a low level mechanism to return -EINTR
> > while most of callers doesn't really want to bother.
> >
>
> That's the point, most callers shouldn't have to, because most callers shouldn't
> be far enough into their operations that -EINTR causes problems.

Agreed, that's a sane pattern to follow so we should convert the
remaining cases, perhaps also auditing all existing
btrfs_start_transaction calls but after a quick look I haven't spotted
anything else than the reloc and drop snapshot.

> We should probably just change btrfs_drop_snapshot to use join, and then have it
> do any space reservation it needs outside of the trans handle. The other option
> is a FLUSH_ALL_UNKILLABLE. Thanks,

I'd rather not push the killable semantics to the flushing state machine
and let it up to the caller context to decide.