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

[David Sterba <dsterba@suse.cz>]

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.