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

[Chandan Rajendra <chandan@linux.vnet.ibm.com>]

On Friday, September 8, 2017 10:25:26 PM IST Eric Sandeen wrote:
> On 9/5/17 12:44 AM, 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.
> > 
> > With these changes, mkfs.xfs now prints error messages when filesystem
> > blocksize (4k) is less than underlying device's physical
> > sectorsize (64k). These messages (printed on stderr) now cause several
> > xfstests to fail on ppc64 machine since xfstests' _filter_mkfs() isn't
> > able to filter out stderr.
> > 
> > Also, the messages themselves describe a possible sub-optimal setup. But
> > the setup is still usable.
> > 
> > Hence this commit removes the calls to fprintf() used to print the
> > messages.
> 
> So, it looks like the loop change is getting reverted, right ... still -
> 
> Although I suggested this change, I'm rethinking it.  I'm not a fan
> of the warning for a default situation; the user can get this warning
> with nothing but a bare mkfs, which is not good IMHO.
> 
> (dchinner OTOH thinks we should warn about this suboptimal situation
> in any case - but I really don't think it's mkfs's job to be warning
> about every suboptimal geometry - there are a lot of them out there!)
> 
> What I'd now propose is that we change this warning into a failure,
> but only if a too-small block size was actually /specified/, i.e.
> bsflag is set.  If we're adjusting sector size based on device geometry
> and /default/ blocksize, I think we should just shut up about it.
> 
> i.e. something like:
> 
>                 if ((blocksize < sectorsize) && (blocksize >= ft.lsectorsize)) {

I agree with your changes from a system administrator's perspective. But
without these messages, the sectorsize change for the loop device would 
most likely not have been noticed.

> 			if (bsflag) {

Just FYI, We should also be checking blflag's value.

>                                 fprintf(stderr,
> _("specified blocksize %d cannot be less than device physical sector size %d\n"),
>                                         blocksize, ft.psectorsize);
>                                 usage();
>                         }
>                         sectorsize = ft.lsectorsize ? ft.lsectorsize :
>                                                       XFS_MIN_SECTORSIZE;
>                 }
> 
> Thoughts?
> 

-- 
chandan