> On Oct. 7, 2015, 5:42 p.m., Justin Gibbs wrote: > > usr/src/uts/common/fs/zfs/dsl_scan.c, lines 1421-1427 > > <https://reviews.csiden.org/r/250/diff/1/?file=17649#file17649line1421> > > > > Wouldn't it be better to merge this logic into dsl_scan_active() so > > that the policy is contained in one place? Something like: > > > > ```c > > if (!dsl_scan_active(scn, INCLUDE_STALLED_SCANS)) > > return; > > ``` > > Justin Gibbs wrote: > Grr. Review board posted a dup and I somehow failed to mark the right > lines... > > George Wilson wrote: > I'm not convinced it would be better since proceeding with a scan when a > scan is not running is not common policy but I don't have a strong opinion > either way. I think if we did change the interface I would prefer to see a > "force" flag or something like that so that it's clear that we're asking for > an exception. Using "include" make it seem like that's the preferred behavior > (at least to me).
I don't feel strongly enough to hold up the review about this, but will expand on my thinking in case it is useful. The original problem was caused by a local attempt to override the global scan policy as defined by dsl_scan_active(), rather than make dsl_scan_active() flexible enough to support this use case. The fix here is to extract more policy from dsl_scan_active() (the spa_shutting_down() check). It seems quite probable to me that a future change or fix to the global scan policy within dsl_scan_active() will either break the use case in dsl_scan_sync() or miss some nuance of how its policy is just like dsl_scan_active(), but not quite. Supporting both policy types within dsl_scan_active() would, I think, significantly reduct this risk. As for the mechanism to inform dsl_scan_active() of the type of policy requested, I'm fine with a boolean or an enum. For illumos I typically advocate for enums based on guidance I received in the past when suggesting the use of the FreeBSD style of adding inline comments next to booleans in call sites to improve readability/provide devloper intent. If clarity is required, the concensus seems to be that an enum is better than a boolean+comment. If you choose to merge the policy into dsl_scan_active() and use an enum, I'm sure you can think of a better value name than the one I picked. :-) Cscope tells me there are currently only two call sites for dsl_scan_active(): one that wants to discount stalled scans, and one that does not. Perhaps your comment about "proceeding with a scan when a scan is not running is not common policy" is that the call site wanting to accout for stalled scans runs more often? Otherwise it seems that both policies are equally useful. - Justin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/250/#review818 ----------------------------------------------------------- On Oct. 7, 2015, 3:02 a.m., Matthew Ahrens wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/250/ > ----------------------------------------------------------- > > (Updated Oct. 7, 2015, 3:02 a.m.) > > > Review request for OpenZFS Developer Mailing List and Christopher Siden. > > > Bugs: 6292 > https://www.illumos.org/issues/6292 > > > Repository: illumos-gate > > > Description > ------- > > 6292 exporting a pool while an async destroy is running can leave entries in > the deferred tree > Reviewed by: Paul Dagnelie <p...@delphix.com> > Reviewed by: Matthew Ahrens <mahr...@delphix.com> > > Original author: George Wilson > > > Diffs > ----- > > usr/src/uts/common/fs/zfs/dsl_scan.c > 6ba5cb6a1c831f681fba16dbc8d25fd8b59b13c9 > > Diff: https://reviews.csiden.org/r/250/diff/ > > > Testing > ------- > > http://jenkins.delphix.com/job/zfs-precommit/3149/ > > > Thanks, > > Matthew Ahrens > >
_______________________________________________ developer mailing list developer@open-zfs.org http://lists.open-zfs.org/mailman/listinfo/developer
