| Message ID | 20220412225806.5647-1-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ksmbd: set fixed sector size to FS_SECTOR_SIZE_INFORMATION | expand |
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); So, this sounds like a great approach, much better than returning 128K. However, it's not at all universally true that 4K is going to be atomic. And min(stfs.f_bsize, 4096) might be problematic too. Is there any better option?? Tom. > info->Flags = cpu_to_le32(SSINFO_FLAGS_ALIGNED_DEVICE | > SSINFO_FLAGS_PARTITION_ALIGNED_ON_DEVICE); > info->ByteOffsetForSectorAlignment = 0;
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; >
On 4/12/2022 11:23 PM, Namjae Jeon wrote: > 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? Where is it guaranteed that the physical storage is 4K?? >> 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. > 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? Tom. > Thanks! >> >> Tom. >> >> >>> info->Flags = cpu_to_le32(SSINFO_FLAGS_ALIGNED_DEVICE | >>> SSINFO_FLAGS_PARTITION_ALIGNED_ON_DEVICE); >>> info->ByteOffsetForSectorAlignment = 0; >> >
2022-04-13 21:25 GMT+09:00, Tom Talpey <tom@talpey.com>: > On 4/12/2022 11:23 PM, Namjae Jeon wrote: >> 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? > > 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; >>> >> >
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); info->Flags = cpu_to_le32(SSINFO_FLAGS_ALIGNED_DEVICE | SSINFO_FLAGS_PARTITION_ALIGNED_ON_DEVICE); info->ByteOffsetForSectorAlignment = 0;
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(-)