On 4/4/24 5:33 PM, Ryusuke Konishi wrote: > On Fri, Apr 5, 2024 at 5:35 AM Eric Sandeen wrote: >> >> On 4/4/24 3:11 PM, Ryusuke Konishi wrote: >>> On Thu, Apr 4, 2024 at 7:12 AM Eric Sandeen wrote: >>>> >>>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx> >>>> --- >>>> >>>> Note: This one was relatively more complex than others, so I would >>>> appreciate review and testing. Basic checks of mounts, various mount >>>> options, and snapshot mounts do seem to work. I may well have missed >>>> something though, as I am not very familiar with nilfs. >>>> >>>> You may want to look closely at the handling of case Opt_err: which >>>> no longer uses nilfs_write_opt() and open-codes the flag change, so >>>> that I can use the enum. If you'd prefer to make 3 independent >>>> Opt_err_XXXZ cases, that would be possible. >>>> >>>> If any of the other changes here are unclear, or problematic, please >>>> let me know. >>>> >>>> Thanks! >>>> -Eric >>> >>> Hi Eric, >>> >>> Thank you! This is one of the modernizations that I thought I had to >>> do with nilfs2. >>> >>> I'm planning on doing a full review later, but when I ran a mount >>> pattern test, the kernel restarted without any messages (probably >>> caused a panic), so I'll give you some quick feedback. >>> >>> The mount pattern that caused the kernel to restart was a simultaneous >>> mount of the current tree and a snapshot, which occurred when the >>> snapshot was mounted and then the current tree was mounted. Something >>> like below: >>> >>> $ sudo losetup /dev/loop0 ./nilfs.iso >>> $ sudo mount -t nilfs2 -o ro,cp=38866 /dev/loop0 /mnt/snapshot >>> $ sudo mount -t nilfs2 /dev/loop0 /mnt/test >>> --> panic >>> >>> Here, 38866 is the snapshot number that can be created with the >>> nilfs-utils "mkcp -s" command or "chcp" command, and the number can be >>> checked with "lscp -s". >>> >>> I have placed the mount test script I used in the following location: >>> >>> https://github.com/konis/nilfs-test-tools/blob/main/test-nilfs-mount.sh >>> >>> The panic occurred in test #17 of that script. >>> >>> I'll also try to track what's going on. >> >> Thanks, I'll look - I was hoping/expecting that you had better tests for >> mount options than I did! ;) >> >> Feel free to debug if you like, but it must be a bug in my patch so >> I'll take ownership of trying to track down the problem and get it to >> pass your test script. > > Got it! > > So I'll try to understand the patch first. Sorry that it's not really possible to break it down into smaller changes. > This test script focuses on reproducing NILFS-specific mount sequences > (such as mounting a snapshot and current tree simultaneously) and > checking the state of user space such as the GC process and utab. > And, is not exhaustive for mount options. > > Looking at the patch, if I come up with test patterns that would be > better to add, I will enhance the test script. Sounds good. So the oops you hit is when a snapshot is mounted first, then the main fs is mounted. My patch does this: /* * Try remount to setup mount states if the current * tree is not mounted and only snapshots use this sb. */ err = nilfs_reconfigure(fc); (in place of nilfs_remount()), and nilfs_reconfigure expects to have fc->root set, which is normally only set up for an actual remount. fc->root is NULL, so that's the oops. I'll see what I can work out. Thanks, -Eric > Thanks, > Ryusuke Konishi > >> >> -Eric >> >>> Thanks, >>> Ryusuke Konishi >> >
