Date: Thu, 15 May 2025 15:58:03 -0600 From: John Nielsen <lists@jnielsen.net> To: Warner Losh <imp@bsdimp.com> Cc: scsi@freebsd.org Subject: Re: isboot: help me understand what CAM is doing Message-ID: <8D8F60A2-98E2-4ED7-AB59-E1B7B3DFD10A@jnielsen.net> In-Reply-To: <CANCZdfpzhu0n1EuX4YsbOkxN_qK2A6gSRfQQnqWLub%2BTPBwEWA@mail.gmail.com> References: <F16B5BD7-4D1F-41F2-8091-508923F09553@jnielsen.net> <CANCZdfpzhu0n1EuX4YsbOkxN_qK2A6gSRfQQnqWLub%2BTPBwEWA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> On May 15, 2025, at 2:24=E2=80=AFPM, Warner Losh <imp@bsdimp.com> = wrote: >=20 > On Thu, May 15, 2025 at 10:30=E2=80=AFAM John Nielsen = <lists@jnielsen.net> wrote: >>=20 >> Hi all- >>=20 >> I=E2=80=99m working on a cosmetic bug in isboot-kmod. There is a = global string called isboot_boot_device which is printed for = informational purposes and also available via the net.isboot.device = sysctl. The string is populated in this function: = https://github.com/jnielsendotnet/isboot/blob/master/src/iscsi.c#L1870. >>=20 >> I don=E2=80=99t know when this changed but historically the string = comparison on = https://github.com/jnielsendotnet/isboot/blob/master/src/iscsi.c#L1904 = would be called once with =E2=80=9Cpass=E2=80=9D and once with =E2=80=9Cda= =E2=80=9D and the global isboot_boot_device would be correctly populated = with e.g. =E2=80=9Cda0=E2=80=9D. >=20 > Yes. We create two peripherals for each device found (well, we always > create pass when pass is in the kernel, and if another driver like da > or cd likes the device, we'll create a periph for that device. >=20 > I've never used this iscsi before... >=20 >> That sometimes happens in the current code as well, but on my test = machine (running 14-STABLE) it is ONLY when I have enabled debug output = (by setting bootverbose or net.isboot.debug to 1 or higher). Otherwise, = the string comparison is called only once with =E2=80=9Cprobe=E2=80=9D = rather than =E2=80=9Cpass=E2=80=9D or =E2=80=9Cda=E2=80=9D. Everything = still works in this case; the disk is found (at da0 or whatever) and = mounted, but the isboot_boot_device is never populated (or populated = with the wrong name like =E2=80=9Cprobe0=E2=80=9D if I mess with what = the string comparison is looking for. >=20 > So what we do is that the scsi XPT layer creates a probe device (whose > name is "probe") for each device that's either scanned or that the SIM > tells XPT exists. This probe device then sends a bunch of SCSI > commands to the device to determine what the device is. Once that's > done, it offers the device to each of the periph drivers, who either > pass on the device, or create a cam_periph for that device. >=20 >> There is no functional change other than debug messages when debug = output is enabled, so I=E2=80=99m guessing this is a race condition. But = since I am still a rank amateur when it comes to kernel programming I = don=E2=80=99t know where else to look. So my questions are twofold: >=20 > Likely we're not proceeding to create the pass or the da device > because the initial commands fail somehow. >=20 >> 1) What is going on here? When does the =E2=80=9Cprobe=E2=80=9D name = show up in ccb.cgdl.periph_name and why doesn=E2=80=99t the loop ever = see =E2=80=9Cda=E2=80=9D or =E2=80=9Cpass=E2=80=9D when it does? = Corollary: does isboot_cam_set_devices() operate in a safe/sane way for = modern CAM? >=20 > Not sure which loop this is, so I can't say. But 'pass' is there while > scsi_xpt is looking at the device, and then the async routines decide > whether to add da or pass devices and then the probe device is > removed. The last two happen in parallel, so there could be a race > there if you are examining the periph lists. >=20 >> =46rom looking at the code, it looks like you may be doing racey = things > by rescanning the device and doing things when the rescan is done. >=20 >> 2) What would be a safer or more reliable way to determine the = correct device name so it can be written to the isboot_boot_device = global variable? >=20 > It should be something like da0. pass isn't going to be a block device > (so you can't boot off of it). cd0 you could boot off of, but nobody > exports their SCSI cd. And it's rare that the boot media is > multi-voliume, so it's unlikely to be da1, etc. and we don't support > any other kind of boot (tape, etc). >=20 > So I'd love to help, but you're currently way too zoomed in on the > problem and assuming that we have more context to what you're trying > to do than I think we have. This makes it hard to know how to help. Thanks Warner. Apologies, let me provide some more context. This isboot = code was written by Daisuke Aaoyama in the FreeBSD 8 days (or earlier). = He stopped updating it several years ago. I have been the port = maintainer for net/isboot-kmod for a long time but am not well-versed in = the code. In 2021 I created the GitHub repository where it lives today = since I needed a place to host patches submitted by others. Since then I = have made some minor improvements (mostly to support my own use cases) = in addition to bringing in other patches to keep the code working in = newer FreeBSD versions (thanks jhb). Ideally this functionality should exist in the base system, but I = don=E2=80=99t know enough to merge the isboot code with the kernel = initiator code that=E2=80=99s already there. In the mean time, I=E2=80=99m= trying to fix issues that I and others have encountered and reported on = the GitHub project. The links in my previous email are to specific line numbers in the code = on the GitHub repository (and possibly off by a few lines due to commits = I=E2=80=99ve made in the mean time=E2=80=A6). Since I did not author = this code and have not otherwise worked much with FreeBSD=E2=80=99s SCSI = subsystem I can=E2=80=99t provide much more context than what is in this = code itself. So if you or someone else would be willing to take a look = I=E2=80=99d be grateful. I=E2=80=99m also trying to learn as I go so I = can be more useful in the future. The other open issue I=E2=80=99m trying to solve is where the CAM rescan = fails to complete entirely. So I=E2=80=99m open to any type of feedback = from =E2=80=9Ctry getting the device name at this point instead=E2=80=9D = to =E2=80=9Cthis line looks race-y because=E2=80=A6=E2=80=9D to =E2=80=9Cs= pend some time reading X or Y=E2=80=9D (but chapter 12 of the developer = handbook and your 2015 CAM scheduling BSDCan talk/slides are already on = my list). Or even =E2=80=9Clook at this function in this driver as a = good example of doing Z=E2=80=9D. Zooming out even farther, getting this functionality into the base = system would be fantastic and eliminate the need for a port or this code = living separately entirely. There=E2=80=99s a patch on Phabricator (not = mine) purporting to do that at https://reviews.freebsd.org/D34477 . If = that=E2=80=99s the way forward I=E2=80=99d love to see progress on it = (and help if I can be useful). If not (or in parallel?), I could make a = diff to bring isboot in to the tree as-is, but it would need careful = review before I would expect it to be merged. And removing the bits that = are redundant with the existing initiator would still want to be done at = some point. In any event, thank you for your attention. JN=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8D8F60A2-98E2-4ED7-AB59-E1B7B3DFD10A>