On 2020-03-11 12:01 a.m., Bart Van Assche wrote:
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.
Most of the stuff there is slow moving data, typically set once at device
creation time. The design predates atomic bitops. I can't see why the
addition of an extra bitfield, whose overwriting is not going to cause
a calamity ***, warrants the proposer having the rewrite this patch.
Doug Gilbert
*** setting offline_already effectively invalidates a lot of other bitfields.
Still, if the offline_already fails to be set, due to a clash
(with what?), then at worst the warning message is repeated and the
patch code tries again to set that bitfield.
