On 2020-03-10 09:44, Ewan D. Milne wrote: > On Mon, 2020-03-09 at 19:02 -0700, Bart Van Assche wrote: >> On 2020-03-09 13:54, Ewan D. Milne wrote: >>> The only purpose of the flag is to try to suppress duplicate messages, >>> in the common case it is a single thread submitting the queued I/O which >>> is going to get rejected. If multiple threads submit I/O there might >>> be duplicated messages but that is not all that critical. Hence the >>> lack of locking on the flag. >> >> My concern is that scsi_prep_state_check() may be called from more than >> one thread at the same time and that bitfield changes are not thread-safe. > > Yes, I agree that the flag is not thread-safe, but I don't think that > is a concern. Because if we get multiple rejecting I/O messages until > the other threads see the flag change we are no worse off than before, > and once the sdev state changes back to SDEV_RUNNING we won't call > scsi_prep_state_check() and examine the flag. Hi Ewan, This is the entire list of bitfields in struct scsi_device: unsigned removable:1; unsigned changed:1; unsigned busy:1; unsigned lockable:1; unsigned locked:1; unsigned borken:1; unsigned disconnect:1; unsigned soft_reset:1; unsigned sdtr:1; unsigned wdtr:1; unsigned ppr:1; unsigned tagged_supported:1; unsigned simple_tags:1; unsigned was_reset:1; unsigned expecting_cc_ua:1; unsigned use_10_for_rw:1; unsigned use_10_for_ms:1; unsigned set_dbd_for_ms:1; unsigned no_report_opcodes:1; unsigned no_write_same:1; unsigned use_16_for_rw:1; unsigned skip_ms_page_8:1; unsigned skip_ms_page_3f:1; unsigned skip_vpd_pages:1; unsigned try_vpd_pages:1; unsigned use_192_bytes_for_3f:1; unsigned no_start_on_add:1; unsigned allow_restart:1; unsigned manage_start_stop:1; unsigned start_stop_pwr_cond:1; unsigned no_uld_attach:1; unsigned select_no_atn:1; unsigned fix_capacity:1; unsigned guess_capacity:1; unsigned retry_hwerror:1; unsigned last_sector_bug:1; unsigned no_read_disc_info:1; unsigned no_read_capacity_16:1; unsigned try_rc_10_first:1; unsigned security_supported:1; unsigned is_visible:1; unsigned wce_default_on:1; unsigned no_dif:1; unsigned broken_fua:1; unsigned lun_in_cdb:1; unsigned unmap_limit_for_ws:1; unsigned rpm_autosuspend:1; If any thread modifies any of these existing bitfields concurrently with scsi_prep_state_check(), one of the two modifications will be lost. That is because compilers implement bitfield changes as follows: new_value = (old_value & ~(1 << ...)) | (1 << ...); If two such assignment statements are executed concurrently, both start from the same 'old_value' and only one of the two changes will happen. I'm concerned that this patch introduces a maintenance problem in the long term. Has it been considered to make 'offline_already' a 32-bits integer variable or a boolean instead of a bitfield? I think such variables can be modified without discarding values written from another thread. Thanks, Bart.
