On Wed, Oct 04, 2017 at 07:07:26AM +1100, Dave Chinner wrote: > On Tue, Oct 03, 2017 at 10:16:04AM -0700, Darrick J. Wong wrote: > > On Tue, Oct 03, 2017 at 07:06:07PM +1100, Dave Chinner wrote: > > > Hi Eric, > > > > > > I've put the latest mkfs refactor code that I have up in place > > > you can pull it from. I've rebased it against the current for-next > > > tree (4.13.1 release) and fixed all the problems that xfstests > > > exposes. The only thing I haven't fixed is xfs/191 that does mkfs > > > command line behaviour verification because the refactored version > > > fixes several problems that the old mkfs didn't handle correctly > > > (e.g. being able to specify certain things like agsize in blocks or > > > sectors). > > > > > > There's a small filter patch needed for xfstests that I'll post in > > > a reply to this pull request that will filter out the new "defaults > > > sourced from ..." output and so prevent spurious xfstests failures. > > > > > > If you want I can tag the branch with a signed tag for you to pull > > > from (same process as Linus prefers) rather than just a branch in a > > > tree. If you'd prefer that I post this as patches instead, then let > > > me know and I'll bomb the list instead. > > > > I had a look at mkfs-refactor. It looks ok to me (I defer to Eric on > > the question of pull req. vs. patchbomb) though I have one question: > > > > calculate_log_size calls max_trans_res, and max_trans_res assembles a > > fake struct xfs_mount in order to call libxfs_log_calc_minimum_size. > > I've fixed a few mkfs bugs over the past couple of years that all stem > > from us forgetting to propagate superblock settings from the > > configuration we're building in main() into the fake xfs_mount->m_sb > > that we use to calculate the minimum log size, which results in a > > disagreement between the kernel and mkfs as to what is the minimum log > > size for a given fs configuration. This disagreement pops up in the > > form of a freshly mkfs'd 500MB filesystem immediately failing to mount. > > > > With this branch applied it looks like we've nearly finished filling out > > the real xfs_mount->m_sb when we call calculate_log_size, so could we > > refactor setup_superblock to set all the non-log superblock fields in > > the real m_sb and then pass that directly into max_trans_res so that we > > can memcpy the real superblock settings into the fake struct xfs_mount? > > Yes, I plan on making further cleanups like that. There are a few > others, like the remaining uses of the global block size and sector > size variables because the parameter structures are not fed into > number conversion functions that use them. > > > Doing that will eliminate a whole class of "we forgot that we have to > > set sb_newfield in setup_superblock /and/ in max_trans_res and now mkfs > > creates broken filesystems" bugs. Even now there are small > > discrepancies between (for example) tr_itruncate.tr_logres in the kernel > > and in mkfs, which make me nervous. AFAICT the discrepancies result in > > mkfs using a minimum log size that is larger than what the kernel > > calculates, so there's no user-visible badness. > > Getting rid of the max_trans_res problem will be good, but it won't > completely fix the problem up. Other nasties in this area that need > further cleanup is the units that stripe configuration are passed > around in, when we store the log stripe unit into the superblock, > documenting what the values in the sb variable are supposed to be, > how sector size and blocksize affects LSU, etc. These were all > things I tripped over that led to similar "why doesn't this > filesystem mount/crash log recovery?" issues. Ok, just making sure it was on your radar. :) --D > > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
