On Fri, Oct 30, 2020 at 01:51:14PM -0400, Brian Foster wrote: > On Fri, Oct 30, 2020 at 11:02:59PM +0800, Gao Xiang wrote: ... > > > > > > That said, this is still quite incomplete in that we can only reduce the > > > size of the tail AG, and if any of that space is in use, we don't > > > currently do anything to try and rectify that. Given that, I'd be a > > > little hesitant to expose this feature as is to production users. IMO, > > > the current kernel feature state could be mergeable but should probably > > > also be buried under XFS_DEBUG until things are more polished. To me, > > > the ideal level of completeness to expose something in production > > > kernels might be something that can 1. relocate used blocks out of the > > > target range and then possibly 2. figure out how to chop off entire AGs. > > > My thinking is that if we never get to that point for whatever > > > reason(s), at least DEBUG mode allows us the flexibility to drop the > > > thing entirely without breaking real users. > > > > Yeah, I also think XFS_DEBUG or another experimential build config > > is needed. > > > > Considering that, I think it would better to seperate into 2 functions > > as Darrick suggested in the next version to avoid too many > > #ifdef XFS_DEBUG #endif hunks. > > > > Another option could be to just retain the existing error checking logic > and wrap it in an ifdef. I.e.: > > #ifndef DEBUG > /* shrink only allowed in debug mode for now ... */ > if (nb < mp->m_sb.sb_dblocks) > return -EINVAL; > #endif > > Then the rest of the function doesn't have to be factored differently > just because of the ifdef. Yeah, on the runtime side, that is ok. Yet the growfs expend common code has been modified due to this patch so it might cause some potential regression (but I don't think for now.) Either way is good to me. Thanks, Gao Xiang > > Brian >
