* [PATCH] ksmbd: set fixed sector size to FS_SECTOR_SIZE_INFORMATION @ 2022-04-12 22:58 Namjae Jeon 2022-04-13 1:40 ` Tom Talpey 0 siblings, 1 reply; 5+ messages in thread From: Namjae Jeon @ 2022-04-12 22:58 UTC (permalink / raw) To: linux-cifs; +Cc: smfrench, hyc.lee, senozhatsky, Namjae Jeon, Christoph Hellwig 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); info->Flags = cpu_to_le32(SSINFO_FLAGS_ALIGNED_DEVICE | SSINFO_FLAGS_PARTITION_ALIGNED_ON_DEVICE); info->ByteOffsetForSectorAlignment = 0; -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ksmbd: set fixed sector size to FS_SECTOR_SIZE_INFORMATION 2022-04-12 22:58 [PATCH] ksmbd: set fixed sector size to FS_SECTOR_SIZE_INFORMATION Namjae Jeon @ 2022-04-13 1:40 ` Tom Talpey 2022-04-13 3:23 ` Namjae Jeon 0 siblings, 1 reply; 5+ messages in thread From: Tom Talpey @ 2022-04-13 1:40 UTC (permalink / raw) To: Namjae Jeon, linux-cifs; +Cc: smfrench, hyc.lee, senozhatsky, Christoph Hellwig 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; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ksmbd: set fixed sector size to FS_SECTOR_SIZE_INFORMATION 2022-04-13 1:40 ` Tom Talpey @ 2022-04-13 3:23 ` Namjae Jeon 2022-04-13 12:25 ` Tom Talpey 0 siblings, 1 reply; 5+ messages in thread From: Namjae Jeon @ 2022-04-13 3:23 UTC (permalink / raw) To: Tom Talpey; +Cc: linux-cifs, smfrench, hyc.lee, senozhatsky, Christoph Hellwig 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; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ksmbd: set fixed sector size to FS_SECTOR_SIZE_INFORMATION 2022-04-13 3:23 ` Namjae Jeon @ 2022-04-13 12:25 ` Tom Talpey 2022-04-13 14:31 ` Namjae Jeon 0 siblings, 1 reply; 5+ messages in thread From: Tom Talpey @ 2022-04-13 12:25 UTC (permalink / raw) To: Namjae Jeon; +Cc: linux-cifs, smfrench, hyc.lee, senozhatsky, Christoph Hellwig 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; >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ksmbd: set fixed sector size to FS_SECTOR_SIZE_INFORMATION 2022-04-13 12:25 ` Tom Talpey @ 2022-04-13 14:31 ` Namjae Jeon 0 siblings, 0 replies; 5+ messages in thread From: Namjae Jeon @ 2022-04-13 14:31 UTC (permalink / raw) To: Tom Talpey; +Cc: linux-cifs, smfrench, hyc.lee, senozhatsky, Christoph Hellwig 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; >>> >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-04-13 14:31 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-12 22:58 [PATCH] ksmbd: set fixed sector size to FS_SECTOR_SIZE_INFORMATION Namjae Jeon 2022-04-13 1:40 ` Tom Talpey 2022-04-13 3:23 ` Namjae Jeon 2022-04-13 12:25 ` Tom Talpey 2022-04-13 14:31 ` Namjae Jeon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.