[haiku-development] Re: Review needed for Sector Size not being reported correctly bug
- From: Ingo Weinhold <ingo_weinhold@xxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Sun, 22 Feb 2015 16:24:38 +0100
On 02/22/2015 02:03 PM, Kushal Singh wrote:
I was working on https://dev.haiku-os.org/ticket/8990 . Initially it
involved making sure that partitions with size > 2TB was not allowed.
However later on it was found out that somewhere in the code "file
system block size" and "block device block size" (aka sector) were being
confused , as the BlockSize returned file system block size instead of
sector size.
I have created a patch for the issue . Here is the link
https://dev.haiku-os.org/attachment/ticket/8990/0001-changes-for-block_size-value-being-sector_size.patch.
The problem was that file system code accidentally overwrote the correct
value ( which was calculated from the geometry of the disk ).
I have tested the patch several times ( with different builds) and no
wrong behavior was observed and the problem has been solved ( no issues
seen on my side) . Since I was unable to get any comments on it in the
bug tracker ( 4 days have passed ) I decided to ask for review here.
So I request someone else to review the patch ( for the bug
https://dev.haiku-os.org/ticket/8990 ) so that it can be closed.
As you probably have noticed the partition_data structure has two
versions of certain fields -- <field> and content_<field>. The reason
for that is that there are two perspectives to look at the partition: 1.
from the point of view of the partitioning system that defines the
partition and 2. from the point of view of the disk system (usually file
system) responsible for the partition's content. E.g. there's a size
field, which specifies how large the partition actually is from the
perspective of the defining partitioning system, and a content_size
field, which specifies the size used for the content (e.g. the size
actually formatted with/used by the file system).
The same goes for type (the type of the partition specified by the
partitioning system) and content_type (the actual type of the contained
disk system), name (the partition's name) and content_name (e.g. the
file system's volume name), parameters (e.g. "partition marked active")
and content_parameters (e.g. "index support enabled").
The block_size field doesn't have a corresponding content_block_size
field. The rationale for this is that partitioning systems don't specify
block sizes for partitions they define. They just have a block size they
use for defining/interpreting partition offsets and sizes. So, in fact,
they have a content block size, just as file systems have a content
block size. Since there is no other block size for a partition, the
field has simply been named block_size instead of content_block_size.
Long story short, the file systems setting partition_data::block_size to
their content block size is correct, or IOW your patch 0001 is incorrect
(BTW in the udf module it also removes a TODO that doesn't concern the
removed line).
Patch 0002 looks OK, save for a small coding style issue ("1 << 32").
Patch 0003 looks OK.
I have only glanced over the comments in the ticket. From that and the
not very helpful ticket description unfortunately I cannot say what the
actual problem is that your patch 0001 is meant to fix. Your comment 17
[1] seems to imply that when uninitializing/reinitializing a partition
the block_size field isn't reset correctly.
ATM the disk device manager initially sets the block_size field of the
root partition. That certainly doesn't harm, but given that it is
actually a content field, the contained disk system (in this case the
intel partitioning system) should always set it in the scan_partition()
and initialize() hooks (like the file system implementations do).
To simplify things, it might be a good idea to add a block_size field to
the disk_device_data structure. This way the root partitioning system
can access the value more easily, if needed.
CU, Ingo
[1] https://dev.haiku-os.org/ticket/8990#comment:17
Other related posts:
