On Wed, Sep 23, 2020 at 07:26:46AM +1000, Dave Chinner wrote:
> On Tue, Sep 22, 2020 at 09:03:28AM -0700, Darrick J. Wong wrote:
> > On Tue, Sep 22, 2020 at 12:44:28PM +0800, Gao Xiang wrote:
> > > On Tue, Sep 22, 2020 at 11:42:49AM +0800, Gao Xiang wrote:
> > > > From: Gao Xiang <hsiangkao@xxxxxxxxxx>
> > > >
> > > > Since commit 1c1c6ebcf52 ("xfs: Replace per-ag array with a radix
> > > > tree"), there is no m_peraglock anymore, so it's hard to understand
> > > > the described situation since per-ag is no longer an array and no
> > > > need to reallocate, call xfs_filestream_flush() in growfs.
> > > >
> > > > In addition, the race condition for shrink feature is quite confusing
> > > > to me currently as well. Get rid of it instead.
> > > >
> > >
> > > (Add some words) I think I understand what the race condition could mean
> > > after shrink fs is landed then, but the main point for now is inconsistent
> > > between code and comment, and there is no infrastructure on shrinkfs so
> > > when shrink fs is landed, the locking rule on filestream should be refined
> > > or redesigned and xfs_filestream_flush() for shrinkfs which was once
> > > deleted by 1c1c6ebcf52 might be restored to drain out in-flight
> > > xfs_fstrm_item for these shrink AGs then.
> > >
> > > From the current code logic, the comment has no use and has been outdated
> > > for years. Keep up with the code would be better IMO to save time.
> >
> > Not being familiar with the filestream code at all, I wonder, what
> > replaced all that stuff? Does that need a comment? I can't really tell
> > at a quick glance what coordinates growfs with filestreams.
>
> The filestream perag state would get trashed by the realloc of the
> perag array that growfs used to do. Hence the filestreams had to be
> flushed before growfs could realloc the array so there was no state
> that could be lost by a grow. The m_peraglock was needed to
> serialise that. Moving to the current perag tree setup meant grow no
> longer reallocated perag structures, so they didn't go away
> transiently and lose state any more, hence none of the flushing or
> perag locking was needed anymore.
>
> Shrink is a different matter altogether. The shrink context is going
> to have to flush the filestreams itself after it makes sure that new
> filestreams can't be created in that AG.....
Yeah, just the suggestion about this comment.
(And as Darrick also mentioned before, maybe just removing the related
stuff for now to avoid confusion would be better... I'm not sure...)
Thanks,
Gao Xiang
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
