this is fine as far as it goes, but the other places that already do "mpi3mr_atomic_dec(&tgtdev->block_io)" should also be converted to use the new mpi3mr_atomic_dec_if_not_zero().

In D49749#1136554, @chandrakanth.patil_broadcom.com wrote:

In D49749#1135942, @chs wrote:

Ok, I didn't realize that "task management" always meant "target reset" here. Not waiting for in-progress commands makes sense in this context. Could you add a comment to the code explaining this?

Another thing that I noticed while looking at this patch was that all the other places that decrement block_io do it in this style:

if (mpi3mr_atomic_read(&tgtdev->block_io) > 0)
        mpi3mr_atomic_dec(&tgtdev->block_io);

[ckp] Agreed, we need to check the block_io > 0 before decrementing block_io, otherwise if other thread has already decremented then the counter may go negative. I will add new patch for that. Thank you.

whereas this one you've added does not check the current value before decrementing.  is there some reason why this new instance doesn't follow the pattern of the others?

note that the pattern of the others also seems wrong... the check of the current value is not atomic with the decrement, so it looks like one thread could check the value and see that it is not zero, another thread could execute one of the paths that sets the current value to zero, and then the first thread could proceed to decrement the value and cause the value to become -1.  is there something that prevents this scenario from occurring?

[ckp] I didnt get how two thread can access the atomic variable? Is this because the v (variable) is non atomic? but atomic_subtract_int() api should take care of atomicity right? if not then please let us know any other way of making the variable and operation atomic. do we need to use this atomic_fetchadd_int() API to achieve atomicity?

#define mpi3mr_atomic_dec(v) atomic_subtract_int(&(v)->val, 1)

Ok, I didn't realize that "task management" always meant "target reset" here. Not waiting for in-progress commands makes sense in this context. Could you add a comment to the code explaining this?

This change blocks new I/Os from being submitted while a task management command is in progress, but wouldn't you need to also wait for any in-progress I/Os to complete before submitting the task management command? I think you would need to wait for those if your goal is to avoid having the firmware processing I/O commands and task management commands together (which is what this change appears to be trying to do).

I tried building with the new patch and found more problems:

in addition the changes you have here already, you'll also need to add the same line that you added to sys/conf/NOTES to sys/amd64/conf/GENERIC and sys/i386/conf/GENERIC.

fix a bug I introduced when changing the calculation of cpi->maxio.

fixed the typo in the subject and review title

I was going to do this this morning but you beat me to it. sorry for the noise.

removed unnecessary roundup() and fixed some PAGE_SIZE vs MPI3MR_4K_PGSZ confusion.

This patch had also tried to resolve some confusion between the OS PAGE_SIZE and the driver's MPI3MR_4K_PGSZ, but I got some cases wrong. I'll fix those in the next version of the patch as well.

There are some console messages printed by the early loading code, but these happen before the regular kernel printf code is initialized, so you can only see them if you enable "options EARLY_PRINTF" in your kernel config and also use "options UART_NS8250_EARLY_PORT=0x3f8" to set which serial port the early printf output should go to. (eg. the 0x3f8 in this example is com1). you also need to boot with "-v" to enable "bootverbose" messages for the ucode early load activity to be printed.

Updated diff for latest review comments.

move the code copied from cpucontrol to a new file so that we can share it between the kernel and cpucontrol.

I added the handling you requested for the loader blob containing all of the amd ucode files concatenated together.

updated patch with requested changes

I agree, this msgbuftrigger thing is rather loosey-goosey. I'm just trying to have all the msgbuf paths achieve a consistent level of loosey-gooseyness.

In D39246#894657, @mckusick wrote:

You have a very valid point. I addressed this by having just a single message come out when running with -p (preen mode). When encountering a filesystem with no depths it prints `UPDATING FILESYSTEM TO TRACK DIRECTORY DEPTH'' then updates all the directory depths. When running in interactive mode and -y mode it asks once `UPDATE FILESYSTEM TO TRACK DIRECTORY DEPTH?'' then just does it. No further reporting happens in either case.

The concern I have with this change is about user interface. With this patch, if someone renames a directory that is the root of a subtree containing other directories and then unmounts the file system cleanly, fsck will report that the file system is now corrupted because all of the dirdepth values for directories in that subtree are wrong. In fact, every existing file system will immediately be reported as corrupted the first time that the new fsck is run. There are millions of people in the world who know what the fsck output for a clean and uncorrupted UFS file system looks like, and that clean fsck output is very easy to recognize because it's very short and always the same (modulo the line with the last-mounted-on path and the line at the end with the block/inode usage and fragmentation summary). After this patch, fsck can produce millions of lines of output for file systems that aren't really corrupted but rather are merely not optimal, and it will no longer be so simple to tell if a file system is corrupted or not by looking at the fsck output.

I was waiting for the UFS fixes to land.

Updated the diff based on review comments.
The suggestion was to only enable vn_read_from_obj() if the fs fragment size is larger than the page size, but it's actually fine to use vn_read_from_obj() as long as the fs block size is greater than or equal to the page size, so I made the check that way.

This makes sense to me.

No, in this example of 16k pages and 4k blocks, the first page would contain the first 4 blocks, the second page would contain the next 4 blocks, etc, so mmap'ing the file works as you would expect.

made requested changes.

It would be good to go ahead and resolve the XXX comments that are currently being added by the patch.

I would characterize this change as: redefine "read_bias==0" from the current meaning of "prefer writes" to instead mean "no read-vs-write bias".
This patch looks correct in implementing that new meaning, though it would be nice if it didn't involve essentially duplicating the code of cam_iosched_get_write() in cam_iosched_next_bio().