[PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Carlos Maiolino]

xfsprogs (mainly mkfs) is using the logical sector size of a volume to initialize
the filesystem, which, even in devices using Advanced Format, it can get a 512
bytes sector size if it is set as the logical sector size.
This patch changes the ioctl to get the physical sector size, independent of the
logical size.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 libxfs/linux.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/libxfs/linux.c b/libxfs/linux.c
index 2e07d54..0bc49dd 100644
--- a/libxfs/linux.c
+++ b/libxfs/linux.c
@@ -39,6 +39,9 @@ static int max_block_alignment;
 #ifndef BLKSSZGET
 # define BLKSSZGET	_IO(0x12,104)
 #endif
+#ifndef BLKPBSZGET
+# define BLKPBSZGET _IO(0x12,123)
+#endif
 
 #ifndef RAMDISK_MAJOR
 #define RAMDISK_MAJOR	1	/* ramdisk major number */
@@ -165,7 +168,7 @@ platform_findsizes(char *path, int fd, long long *sz, int *bsz)
 		*sz = (long long)tmpsize;
 	}
 
-	if (ioctl(fd, BLKSSZGET, bsz) < 0) {
+	if (ioctl(fd, BLKPBSZGET, bsz) < 0) {
 		fprintf(stderr, _("%s: warning - cannot get sector size "
 				"from block device %s: %s\n"),
 			progname, path, strerror(errno));
-- 
1.7.6.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Carlos Maiolino]

On Thu, Nov 24, 2011 at 05:20:51PM -0200, Carlos Maiolino wrote:
> xfsprogs (mainly mkfs) is using the logical sector size of a volume to initialize
> the filesystem, which, even in devices using Advanced Format, it can get a 512
> bytes sector size if it is set as the logical sector size.
> This patch changes the ioctl to get the physical sector size, independent of the
> logical size.
> 

Just as information, this patch proposal does not change the behaviour of mkfs in case the 
user is using libblkid, which in case, mkfs will take advantage of libblkid to retrieve disk
topology and information.
I'm not sure if libblkid is the best way to retrieve the device sector's size here, since 
this does not provide a way to retrive the physical sector size, only the logical size, but 
I can be very wrong.

-- 
--Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Dave Chinner]

On Thu, Nov 24, 2011 at 05:50:42PM -0200, Carlos Maiolino wrote:
> On Thu, Nov 24, 2011 at 05:20:51PM -0200, Carlos Maiolino wrote:
> > xfsprogs (mainly mkfs) is using the logical sector size of a volume to initialize
> > the filesystem, which, even in devices using Advanced Format, it can get a 512
> > bytes sector size if it is set as the logical sector size.
> > This patch changes the ioctl to get the physical sector size, independent of the
> > logical size.
> > 
> 
> Just as information, this patch proposal does not change the behaviour of mkfs in case the 
> user is using libblkid, which in case, mkfs will take advantage of libblkid to retrieve disk
> topology and information.
> I'm not sure if libblkid is the best way to retrieve the device sector's size here, since 
> this does not provide a way to retrive the physical sector size, only the logical size, but 
> I can be very wrong.

If libblkid exports the PBS (physical block size) as exposed in
/sys/block/<dev>/queue/physical_block_size, then we should be able
to get it.

However, the issue in my mind is not whether it is supported, but
what is the effect of making this change? The filesystem relies on
the fact that the minimum guaranteed unit of atomic IO is a sector,
not the PBS of the underlying disk. What guarantees do we have when
do a PBS sized IO is doesn't get torn into sector sized IOs by the
drive and hence only partitially completed on failure?  Indeed, if
the filesystem is sector unaligned, it is -guaranteed- to have PBS
sized IOs torn apart by the hardware....

i.e. do we have any guarantee at all that a PBS sized IO will either
wholly complete or wholly fail when PBS != sector size? And if not,
why is this a change we should make given it appears to me to
violate a fundamental assumption of the filesystem design?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Eric Sandeen]

On 11/26/11 7:06 PM, Dave Chinner wrote:
> On Thu, Nov 24, 2011 at 05:50:42PM -0200, Carlos Maiolino wrote:
>> On Thu, Nov 24, 2011 at 05:20:51PM -0200, Carlos Maiolino wrote:
>>> xfsprogs (mainly mkfs) is using the logical sector size of a volume to initialize
>>> the filesystem, which, even in devices using Advanced Format, it can get a 512
>>> bytes sector size if it is set as the logical sector size.
>>> This patch changes the ioctl to get the physical sector size, independent of the
>>> logical size.
>>>
>>
>> Just as information, this patch proposal does not change the behaviour of mkfs in case the 
>> user is using libblkid, which in case, mkfs will take advantage of libblkid to retrieve disk
>> topology and information.
>> I'm not sure if libblkid is the best way to retrieve the device sector's size here, since 
>> this does not provide a way to retrive the physical sector size, only the logical size, but 
>> I can be very wrong.
> 
> If libblkid exports the PBS (physical block size) as exposed in
> /sys/block/<dev>/queue/physical_block_size, then we should be able
> to get it.
> 
> However, the issue in my mind is not whether it is supported, but
> what is the effect of making this change? The filesystem relies on
> the fact that the minimum guaranteed unit of atomic IO is a sector,
> not the PBS of the underlying disk. What guarantees do we have when
> do a PBS sized IO is doesn't get torn into sector sized IOs by the
> drive and hence only partitially completed on failure?  Indeed, if
> the filesystem is sector unaligned, it is -guaranteed- to have PBS
> sized IOs torn apart by the hardware....
> 
> i.e. do we have any guarantee at all that a PBS sized IO will either
> wholly complete or wholly fail when PBS != sector size? And if not,
> why is this a change we should make given it appears to me to
> violate a fundamental assumption of the filesystem design?

I had the expectation that physical block size WAS the fundamental/atomic
IO size for the disk, and anything smaller required read/modify/write.
So I made this suggestion (and I think hch concurred) so that we weren't
doing log IOs which required RMW & translation.

i.e. for a 4k physical / 512 logical disk - wouldn't we want to choose
4k sectors?

Ok, if we have mismanaged the alignment and aligned to logical, not
physical, then I guess there would be an issue... but at that point
we've already messed up (though not catastrophically I guess)...

-Eric

> Cheers,
> 
> Dave.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Dave Chinner]

On Sun, Nov 27, 2011 at 05:05:23PM -0600, Eric Sandeen wrote:
> On 11/26/11 7:06 PM, Dave Chinner wrote:
> > On Thu, Nov 24, 2011 at 05:50:42PM -0200, Carlos Maiolino wrote:
> >> On Thu, Nov 24, 2011 at 05:20:51PM -0200, Carlos Maiolino wrote:
> >>> xfsprogs (mainly mkfs) is using the logical sector size of a volume to initialize
> >>> the filesystem, which, even in devices using Advanced Format, it can get a 512
> >>> bytes sector size if it is set as the logical sector size.
> >>> This patch changes the ioctl to get the physical sector size, independent of the
> >>> logical size.
> >>>
> >>
> >> Just as information, this patch proposal does not change the behaviour of mkfs in case the 
> >> user is using libblkid, which in case, mkfs will take advantage of libblkid to retrieve disk
> >> topology and information.
> >> I'm not sure if libblkid is the best way to retrieve the device sector's size here, since 
> >> this does not provide a way to retrive the physical sector size, only the logical size, but 
> >> I can be very wrong.
> > 
> > If libblkid exports the PBS (physical block size) as exposed in
> > /sys/block/<dev>/queue/physical_block_size, then we should be able
> > to get it.
> > 
> > However, the issue in my mind is not whether it is supported, but
> > what is the effect of making this change? The filesystem relies on
> > the fact that the minimum guaranteed unit of atomic IO is a sector,
> > not the PBS of the underlying disk. What guarantees do we have when
> > do a PBS sized IO is doesn't get torn into sector sized IOs by the
> > drive and hence only partitially completed on failure?  Indeed, if
> > the filesystem is sector unaligned, it is -guaranteed- to have PBS
> > sized IOs torn apart by the hardware....
> > 
> > i.e. do we have any guarantee at all that a PBS sized IO will either
> > wholly complete or wholly fail when PBS != sector size? And if not,
> > why is this a change we should make given it appears to me to
> > violate a fundamental assumption of the filesystem design?
> 
> I had the expectation that physical block size WAS the fundamental/atomic
> IO size for the disk, and anything smaller required read/modify/write.
> So I made this suggestion (and I think hch concurred) so that we weren't
> doing log IOs which required RMW & translation.

A RMW cycle does not mean the IO is not atomic. The write to disk
will still be atomic, regardless of the read that ovvurred before.

> i.e. for a 4k physical / 512 logical disk - wouldn't we want to choose
> 4k sectors?
> 
> Ok, if we have mismanaged the alignment and aligned to logical, not
> physical, then I guess there would be an issue... but at that point
> we've already messed up (though not catastrophically I guess)...

That's where I'm concerned - if alignment is screwed because the FS
is 512B sector aligned (because something read the logical sector size),
then using a 4k sector will result in torn writes because every 4k
sector write is potentially made up of 2 4k write IOs, not 1.

That's my concern - using the logical 512b sector size is -always-
safe, but using the 4k physical block size is only safe if
everything under the filesystem has detected and used the physical
block size of the disk for alignment and sector sizes...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Christoph Hellwig]

On Mon, Nov 28, 2011 at 10:50:51AM +1100, Dave Chinner wrote:
> > I had the expectation that physical block size WAS the fundamental/atomic
> > IO size for the disk, and anything smaller required read/modify/write.
> > So I made this suggestion (and I think hch concurred) so that we weren't
> > doing log IOs which required RMW & translation.
> 
> A RMW cycle does not mean the IO is not atomic. The write to disk
> will still be atomic, regardless of the read that ovvurred before.

I would not trust ATA disk to get this right generally.

> > Ok, if we have mismanaged the alignment and aligned to logical, not
> > physical, then I guess there would be an issue... but at that point
> > we've already messed up (though not catastrophically I guess)...
> 
> That's where I'm concerned - if alignment is screwed because the FS
> is 512B sector aligned (because something read the logical sector size),
> then using a 4k sector will result in torn writes because every 4k
> sector write is potentially made up of 2 4k write IOs, not 1.

Disks that implement the ATA level required to tell us about the
physical blocksize also have the alignment offset information in the
IDENTIFY data to tell us about aligned shifts.  But I haven't seen a
single one with a non-zero aligned offset in the wild.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Martin K. Petersen]

>>>>> "Dave" == Dave Chinner <david@fromorbit.com> writes:

>> Ok, if we have mismanaged the alignment and aligned to logical, not
>> physical, then I guess there would be an issue... but at that point
>> we've already messed up (though not catastrophically I guess)...

Dave> That's where I'm concerned - if alignment is screwed because the
Dave> FS is 512B sector aligned (because something read the logical
Dave> sector size), then using a 4k sector will result in torn writes
Dave> because every 4k sector write is potentially made up of 2 4k write
Dave> IOs, not 1.

There's another inherent failure scenario with 512b logical / 4096b
physical. If you write in 512-byte multiples and experience a medium
error you can lose the sibling logical blocks within that physical
block. You'll get an I/O error back but there are no means to
communicate that you have also lost blocks that were not part of your
write request. So if you use 512-byte entries in the journal and get a
write error you should at the very minimum consider adjacent entries
inside a 4KB window suspect.


Dave> That's my concern - using the logical 512b sector size is -always-
Dave> safe, but using the 4k physical block size is only safe if
Dave> everything under the filesystem has detected and used the physical
Dave> block size of the disk for alignment and sector sizes...

You should always take alignment into account.

And while Christoph is right that (thankfully) nobody ended up shipping
drives with 1-alignment by default, most 512e drives have the alignment
jumper and some people actually use it.

-- 
Martin K. Petersen	Oracle Linux Engineering

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Eric Sandeen]

On 11/28/11 10:08 AM, Martin K. Petersen wrote:
>>>>>> "Dave" == Dave Chinner <david@fromorbit.com> writes:
> 
>>> Ok, if we have mismanaged the alignment and aligned to logical, not
>>> physical, then I guess there would be an issue... but at that point
>>> we've already messed up (though not catastrophically I guess)...
> 
> Dave> That's where I'm concerned - if alignment is screwed because the
> Dave> FS is 512B sector aligned (because something read the logical
> Dave> sector size), then using a 4k sector will result in torn writes
> Dave> because every 4k sector write is potentially made up of 2 4k write
> Dave> IOs, not 1.
> 
> There's another inherent failure scenario with 512b logical / 4096b
> physical. If you write in 512-byte multiples and experience a medium
> error you can lose the sibling logical blocks within that physical
> block. You'll get an I/O error back but there are no means to
> communicate that you have also lost blocks that were not part of your
> write request. So if you use 512-byte entries in the journal and get a
> write error you should at the very minimum consider adjacent entries
> inside a 4KB window suspect.
> 
> 
> Dave> That's my concern - using the logical 512b sector size is -always-
> Dave> safe, but using the 4k physical block size is only safe if
> Dave> everything under the filesystem has detected and used the physical
> Dave> block size of the disk for alignment and sector sizes...
> 
> You should always take alignment into account.
> 
> And while Christoph is right that (thankfully) nobody ended up shipping
> drives with 1-alignment by default, most 512e drives have the alignment
> jumper and some people actually use it.

It seems that we should be checking for any alignment offsets in libxfs
then, too; if there IS an offset, then perhaps 4k is the wrong answer,
(perhaps there is no right answer) but if there is NO offset, 4k should
be the right choice, yes?

And if the drive is broken then c'est la vie?

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Martin K. Petersen]

>>>>> "Eric" == Eric Sandeen <sandeen@sandeen.net> writes:

Eric> It seems that we should be checking for any alignment offsets in
Eric> libxfs then, too; if there IS an offset, then perhaps 4k is the
Eric> wrong answer, (perhaps there is no right answer) but if there is
Eric> NO offset, 4k should be the right choice, yes?

In most cases the partitioning/DM tools should give you a 0 offset. But
it would a good idea to at least print a warning if lbs != pbs and
offset > 0.


Eric> And if the drive is broken then c'est la vie?

Yes :)


FWIW, the reason 4KB lbs drives are having a revival in the is that
there is not a lot of confidence in 512e for the enterprise. Many
vendors won't support them in servers due to correctness concerns and
lack of performance predictability.

-- 
Martin K. Petersen	Oracle Linux Engineering

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Eric Sandeen]

On 11/29/11 11:15 AM, Martin K. Petersen wrote:
>>>>>> "Eric" == Eric Sandeen <sandeen@sandeen.net> writes:
> 
> Eric> It seems that we should be checking for any alignment offsets in
> Eric> libxfs then, too; if there IS an offset, then perhaps 4k is the
> Eric> wrong answer, (perhaps there is no right answer) but if there is
> Eric> NO offset, 4k should be the right choice, yes?
> 
> In most cases the partitioning/DM tools should give you a 0 offset. But
> it would a good idea to at least print a warning if lbs != pbs and
> offset > 0.

Right, Dave's concern was for when the partitioning tools didn't do the
job, we don't want to break fs consistency guarantees...

Dave, does checking for an offset before choosing 4k sectors seem
sufficient to you?

> 
> Eric> And if the drive is broken then c'est la vie?
> 
> Yes :)
> 
> 
> FWIW, the reason 4KB lbs drives are having a revival in the is that
> there is not a lot of confidence in 512e for the enterprise. Many
> vendors won't support them in servers due to correctness concerns and
> lack of performance predictability.

Imagine.  :)

-Eric


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Dave Chinner]

On Tue, Nov 29, 2011 at 11:38:33AM -0600, Eric Sandeen wrote:
> On 11/29/11 11:15 AM, Martin K. Petersen wrote:
> >>>>>> "Eric" == Eric Sandeen <sandeen@sandeen.net> writes:
> > 
> > Eric> It seems that we should be checking for any alignment offsets in
> > Eric> libxfs then, too; if there IS an offset, then perhaps 4k is the
> > Eric> wrong answer, (perhaps there is no right answer) but if there is
> > Eric> NO offset, 4k should be the right choice, yes?
> > 
> > In most cases the partitioning/DM tools should give you a 0 offset. But
> > it would a good idea to at least print a warning if lbs != pbs and
> > offset > 0.
> 
> Right, Dave's concern was for when the partitioning tools didn't do the
> job, we don't want to break fs consistency guarantees...
> 
> Dave, does checking for an offset before choosing 4k sectors seem
> sufficient to you?

Yes, especially if combined with Christoph's comments about ensure
the "-f" flag is needed to make a filesystem on an unaligned config.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Carlos Maiolino]

Ok, looks like we have a definition here about what mkfs should do in regards of the
lbs/pbs. 

I'll be working on a patch to it.

Is there any other thing I should pay attention besides what have been discussed here?
I'll send a patch as soon as I have it :-)

On Wed, Nov 30, 2011 at 11:19:27AM +1100, Dave Chinner wrote:
> On Tue, Nov 29, 2011 at 11:38:33AM -0600, Eric Sandeen wrote:
> > On 11/29/11 11:15 AM, Martin K. Petersen wrote:
> > >>>>>> "Eric" == Eric Sandeen <sandeen@sandeen.net> writes:
> > > 
> > > Eric> It seems that we should be checking for any alignment offsets in
> > > Eric> libxfs then, too; if there IS an offset, then perhaps 4k is the
> > > Eric> wrong answer, (perhaps there is no right answer) but if there is
> > > Eric> NO offset, 4k should be the right choice, yes?
> > > 
> > > In most cases the partitioning/DM tools should give you a 0 offset. But
> > > it would a good idea to at least print a warning if lbs != pbs and
> > > offset > 0.
> > 
> > Right, Dave's concern was for when the partitioning tools didn't do the
> > job, we don't want to break fs consistency guarantees...
> > 
> > Dave, does checking for an offset before choosing 4k sectors seem
> > sufficient to you?
> 
> Yes, especially if combined with Christoph's comments about ensure
> the "-f" flag is needed to make a filesystem on an unaligned config.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

-- 
--Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Greg Freemyer]

On Mon, Nov 28, 2011 at 11:08 AM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
> And while Christoph is right that (thankfully) nobody ended up shipping
> drives with 1-alignment by default, most 512e drives have the alignment
> jumper and some people actually use it.

I've seen at least one of my clients use the jumper.  It was my first
realization that jumper actually "moves" stuff on the disk.

I realize its a virtual move, but data read from sector 63 with the
jumper in place is not the same data that is read from sector 63 with
the jumper removed.

It caught me off guard.  The fix in this case was just to re-install
the jumper like the client had it originally.

Greg

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] libxfs: Get Physical Sector Size instead of Logical Sector size

[Christoph Hellwig]

On Sun, Nov 27, 2011 at 05:05:23PM -0600, Eric Sandeen wrote:
> > i.e. do we have any guarantee at all that a PBS sized IO will either
> > wholly complete or wholly fail when PBS != sector size? And if not,
> > why is this a change we should make given it appears to me to
> > violate a fundamental assumption of the filesystem design?
> 
> I had the expectation that physical block size WAS the fundamental/atomic
> IO size for the disk, and anything smaller required read/modify/write.
> So I made this suggestion (and I think hch concurred) so that we weren't
> doing log IOs which required RMW & translation.

Yes, that is how it is defined. 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs