2022-04-13 21:25 GMT+09:00, Tom Talpey <tom@xxxxxxxxxx>: > On 4/12/2022 11:23 PM, Namjae Jeon wrote: >> 2022-04-13 10:40 GMT+09:00, Tom Talpey <tom@xxxxxxxxxx>: >>> 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@xxxxxx> >>>> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx> >>>> --- >>>> 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? > > Where is it guaranteed that the physical storage is 4K?? ->s_blocksize will be set to 4K on 4K native. If you check v2 patch, the sector size is set to min(->s_blocksize, 4096). > >>> 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). > > And I agree. Did you read the discussion in MS-FSCC by the way? > >> A client can interpret this field as the unit for which NTFS guarantees an >> atomic >> operation. NTFS calculates the value of this field as follows: >> Retrieve the physical sector size the device reports for atomicity, and >> store in x. >> Validate that the value x is greater than or equal to the logical sector >> size. If it is not, set x to the >> logical sector size. >> Validate that the value x is a power of two. If it is not, set x to the >> logical sector size. >> Validate that the value x is less than or equal to the system page size >> defined in [MS-FSA] section >> 2.1.1.1. If it is not, set x to the system page size defined in [MS-FSA] >> section 2.1.1.1. > > That's just the Windows/NTFS approach, of course. value x is ->s_blocksize in NTFS of linux kernel. > >> So we need to select fixed size as between 512B ~ 4KB. I think the v2 >> patch looks a bit better... > > "A bit better"? So what's the actual fix going to be? I think that setting it to fixed logical 512B, physical 4K can report the wrong sector size for 4K native storage. Because it has to be logical 4K, physical 4K. ->s_blocksize is probably a good alternative, set 4K as the maximum sector size for unusual ->s_blocksize like ZFS. Because I haven't seen a device with hw sector size larger than 4K. > > Tom. > >> Thanks! >>> >>> Tom. >>> >>> >>>> info->Flags = cpu_to_le32(SSINFO_FLAGS_ALIGNED_DEVICE | >>>> SSINFO_FLAGS_PARTITION_ALIGNED_ON_DEVICE); >>>> info->ByteOffsetForSectorAlignment = 0; >>> >> >
