Re: [PATCH 2/7] scsi: target: Add atomic se_device fields

[michael.christie@oracle.com]

On 7/23/25 10:10 AM, michael.christie@oracle.com wrote:
> On 7/23/25 6:28 AM, John Garry wrote:
>> On 20/07/2025 23:15, Mike Christie wrote:
>>> On 5/14/25 5:26 AM, John Garry wrote:
>>>> On 08/10/2024 03:03, Mike Christie wrote:
>>>>> This adds atomic fields to the se_device and exports them in configfs.
>>>>>
>>>>> Initially only target_core_iblock will be supported and we will inherit
>>>>> all the settings from the block layer except the alignment which
>>>>> userspace
>>>>> will set.
>>>>>
>>>>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>>>>> ---
>>>>>    drivers/target/target_core_configfs.c | 42 ++++++++++++++++++++++
>>>>> +++++
>>>>>    include/target/target_core_base.h     |  6 ++++
>>>>>    2 files changed, 48 insertions(+)
>>>>>
>>>>> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/
>>>>> target_core_configfs.c
>>>>> index c40217f44b1b..f3c7da650f65 100644
>>>>> --- a/drivers/target/target_core_configfs.c
>>>>> +++ b/drivers/target/target_core_configfs.c
>>>>> @@ -578,6 +578,12 @@ DEF_CONFIGFS_ATTRIB_SHOW(unmap_zeroes_data);
>>>>>    DEF_CONFIGFS_ATTRIB_SHOW(max_write_same_len);
>>>>>    DEF_CONFIGFS_ATTRIB_SHOW(emulate_rsoc);
>>>>>    DEF_CONFIGFS_ATTRIB_SHOW(submit_type);
>>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_supported);
>>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_alignment);
>>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_len);
>>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_granularity);
>>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_with_boundary);
>>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_boundary);
>>>>>      #define DEF_CONFIGFS_ATTRIB_STORE_U32(_name)                \
>>>>>    static ssize_t _name##_store(struct config_item *item, const char
>>>>> *page,\
>>>>> @@ -1266,6 +1272,30 @@ static ssize_t submit_type_store(struct
>>>>> config_item *item, const char *page,
>>>>>        return count;
>>>>>    }
>>>>>    +static ssize_t atomic_alignment_store(struct config_item *item,
>>>>> +                      const char *page, size_t count)
>>>>
>>>> What is special about alignment that we need to be able to configure
>>>> this?
>>>>
>>>> Won't that be derived from the block device queue_limits (like
>>>> granularity)?
>>>
>>> What if you are not using a physical block device like using LIO's
>>> ramdisk mode
>>> to perform testing?
>>
>> But would this non-physical device still need to support atomics? I
>> would assume so.
> 
> It depends. For just a testing mode not necessarily. It can be relaxed
> so you can test different scenarios like you want to test when atomics
> are not supported correctly. For real support yes.
> 
> But just to be clear for either case, there won't always be a
> request_queue/block_device. For example, rd and tcmu don't use
> target_configure_write_atomic_from_bdev, so the atomic values can be
> whatever they want to support.

I think I misunderstood the last comment.

I'm trying to make sure I can support a broken device for testing so I
tried to make as flexible as possible.

With the code as is though I think your concern in the last comment is
that a user could set some valid settings, use rd/tcmu, and think they
were safe when they are not. If so, then I agree that can happen.

Or are you saying even for broken device simulation that we will never
need to set values like alignment above?