Re: [PATCH] ksmbd: set fixed sector size to FS_SECTOR_SIZE_INFORMATION

[Namjae Jeon <linkinjeon@kernel.org>]

2022-04-13 10:40 GMT+09:00, Tom Talpey <tom@talpey.com>:
> On 4/12/2022 6:58 PM, Namjae Jeon wrote:
>> Currently ksmbd is using ->f_bsize from vfs_statfs() as sector size.
>> If fat/exfat is a local share, ->f_bsize is a cluster size that is too
>> large to be used as a sector size. Sector sizes larger than 4K cause
>> problem occurs when mounting an iso file through windows client.
>>
>> The error message can be obtained using Mount-DiskImage command,
>>   the error is:
>> "Mount-DiskImage : The sector size of the physical disk on which the
>> virtual disk resides is not supported."
>>
>> This patch reports fixed sector size as 512B logical/4K physical to
>> windows client to avoid poking into the block device.
>>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   fs/ksmbd/smb2pdu.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index 62cc0f95ab87..28ff7c058bc4 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -4998,12 +4998,11 @@ static int smb2_get_info_filesystem(struct
>> ksmbd_work *work,
>>
>>   		info = (struct smb3_fs_ss_info *)(rsp->Buffer);
>>
>> -		info->LogicalBytesPerSector = cpu_to_le32(stfs.f_bsize);
>> -		info->PhysicalBytesPerSectorForAtomicity =
>> -				cpu_to_le32(stfs.f_bsize);
>> -		info->PhysicalBytesPerSectorForPerf = cpu_to_le32(stfs.f_bsize);
>> +		info->LogicalBytesPerSector = cpu_to_le32(512);
>> +		info->PhysicalBytesPerSectorForAtomicity = cpu_to_le32(4096);
>> +		info->PhysicalBytesPerSectorForPerf = cpu_to_le32(4096);
>>   		info->FSEffPhysicalBytesPerSectorForAtomicity =
>> -				cpu_to_le32(stfs.f_bsize);
>> +				cpu_to_le32(4096);
>
Hi Tom,
> So, this sounds like a great approach, much better than returning 128K.
Thanks:)
>
> However, it's not at all universally true that 4K is going to be atomic.
Could you please elaborate more ? Is the atomic size not 4K for 4K
native storage?

> And min(stfs.f_bsize, 4096) might be problematic too. Is there any better
> option??
There is no option than to poke into block layer, but Christoph
pointed out that this will also give the wrong answer for file systems
with multiple device support (btrfs, f2fs, xfs).
So we need to select fixed size as between 512B ~ 4KB. I think the v2
patch looks a bit better...

Thanks!
>
> Tom.
>
>
>>   		info->Flags = cpu_to_le32(SSINFO_FLAGS_ALIGNED_DEVICE |
>>   				    SSINFO_FLAGS_PARTITION_ALIGNED_ON_DEVICE);
>>   		info->ByteOffsetForSectorAlignment = 0;
>