Re: [PATCH] mkfs: Remove messages printed when blocksize < physical sectorsize

[Omar Sandoval <osandov@osandov.com>]

On Wed, Sep 06, 2017 at 09:24:49AM +1000, Dave Chinner wrote:
> On Tue, Sep 05, 2017 at 03:18:51PM -0700, Omar Sandoval wrote:
> > On Wed, Sep 06, 2017 at 08:06:31AM +1000, Dave Chinner wrote:
> > > On Tue, Sep 05, 2017 at 09:00:26AM -0600, Jens Axboe wrote:
> > > > On 09/05/2017 01:37 AM, Chandan Rajendra wrote:
> > > > > On Tuesday, September 5, 2017 12:12:08 PM IST Omar Sandoval wrote:
> > > > >> On Mon, Sep 04, 2017 at 11:31:39PM -0700, Christoph Hellwig wrote:
> > > > >>> On Tue, Sep 05, 2017 at 11:14:42AM +0530, Chandan Rajendra wrote:
> > > > >>>> Linux kernel commit 6c6b6f28b3335fd85ec833ee0005d9c9dca6c003 (loop: set
> > > > >>>> physical block size to PAGE_SIZE) now sets PAGE_SIZE as the default
> > > > >>>> physical sector size of loop devices. On ppc64, this causes loop devices
> > > > >>>> to have 64k as the physical sector size.
> > > > >>>
> > > > >>> Eek.  We'll need to revert the loop change ASAP!
> > > > >>
> > > > >> Most annoying patch series ever. It hasn't made it to Linus' tree yet,
> > > > >> right? We can revert (although the later change depends on that), fold
> > > > >> in a fix, or apply a fix on top of it, whatever Jens prefers.
> > > > >>
> > > > >>
> > > > > 
> > > > > My bad. 6c6b6f28b3335fd85ec833ee0005d9c9dca6c003 is the commit id from 
> > > > > Linux-next. I don't see this commit in Linus's git tree.
> > > > 
> > > > Right, it's only queued up, and scheduled for the 2nd part of the
> > > > block changes for 4.14. It should have been PAGE_CACHE_SIZE, but
> > > > we don't have that anymore...
> > > 
> > > But PAGE_CACHE_SIZE was equal to PAGE_SIZE, so that would have been
> > > wrong, too.
> > > 
> > > I just don't see why this is necessary, given that buffered IO
> > > through the upper filesystem will already be doing page
> > > sized/aligned IO where possible (because that's what the page cache
> > > does!). And for direct IO the loop device should just export the
> > > underlying host filesystem logical/physical sector sizes, which
> > > should be optimal for the backing storage to begin with.
> > > 
> > > Someone want to enlighten me as to what problem is being solved
> > > here?
> > 
> > I already sent a patch to fix this:
> > https://marc.info/?l=linux-block&m=150464670510301&w=2
> > 
> > Loop was using the default physical block size of 512, which is a lie
> > according to the definition of the physical block size:
> 
> Loop devices are special. They /have to lie/ because they are the
> only mechanism we have for mounting image files with sector sizes
> smaller than the host storage sizes.  i.e. loop devices require
> compatibility and flexibility first and so need to default to 
> "most compatible" behaviour, not "most performant".

And that's why the logical block size is 512 by default. The physical
block size doesn't have any bearing on what you can do with the device,
it's a _hint_ for the application. But again, I'm just being overly
pedantic, I should've checked how tools were actually using the physical
block size.

> If you're really "sick of these stupid changes" then perhaps you
> should consider cc'ing linux-fsdevel for loop device changes so
> filesystem developers have a chance to catch these filesystem side
> problems in new loopdev code before it's merged anywhere....

Yup, loop changes haven't been going outside of linux-block but at least
this one should have gone to fsdevel, sorry.

> FWIW, if you're going to set some kind of optimal default for the
> loop devices then - like all other stacked block devices - the
> defaults need to be pulled from the underlying storage (i.e. the
> filesystem that hosts the file) rather than making them up out of
> thin air.

Not worth the trouble for now, I'll just leave this alone now.