| Message ID | 20170905054442.28615-1-chandan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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! -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! And, FWIW, making the warning go away if probably a bad idea, because XFS only supports devices with sector sizes up to 32k: #define XFS_MIN_SECTORSIZE_LOG 9 /* i.e. 512 bytes */ #define XFS_MAX_SECTORSIZE_LOG 15 /* i.e. 32768 bytes */ And so it should be warning about devices trying to tell it to use something larger.... Cheers, Dave.
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.
On 9/5/17 1:44 AM, Dave Chinner 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! > > And, FWIW, making the warning go away if probably a bad idea, > because XFS only supports devices with sector sizes up > to 32k: Well, TBH removing this warning was my suggestion, because it's automatically fixing values that weren't specified by the user in the first place. First preference is physical sector size, then fallback to logical but it doesn't need to be noisy about it. > #define XFS_MIN_SECTORSIZE_LOG 9 /* i.e. 512 bytes */ > #define XFS_MAX_SECTORSIZE_LOG 15 /* i.e. 32768 bytes */ > > And so it should be warning about devices trying to tell it to use > something larger.... As long as the logical sector size is small enough, it seems like a silent adjustment is probably ok, no? -Eric > Cheers, > > Dave. > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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... Omar, are you sending a patch to fix this up?
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? Cheers, Dave.
On Tue, Sep 05, 2017 at 09:17:42AM -0500, Eric Sandeen wrote: > > > On 9/5/17 1:44 AM, Dave Chinner 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! > > > > And, FWIW, making the warning go away if probably a bad idea, > > because XFS only supports devices with sector sizes up > > to 32k: > > Well, TBH removing this warning was my suggestion, because it's > automatically fixing values that weren't specified by the user in > the first place. First preference is physical sector size, then > fallback to logical but it doesn't need to be noisy about it. > > > #define XFS_MIN_SECTORSIZE_LOG 9 /* i.e. 512 bytes */ > > #define XFS_MAX_SECTORSIZE_LOG 15 /* i.e. 32768 bytes */ > > > > And so it should be warning about devices trying to tell it to use > > something larger.... > > As long as the logical sector size is small enough, it seems like a > silent adjustment is probably ok, no? Think 512e drives. Doing 512 byte sector IO is possible, but slow. Someone might actually want to avoid that by having the filesystem use 4k sector sizes. However, if for some reason, mkfs selects 512 byte sectors (the logical size) rather than 4k sector size, then shouldn't we be telling the user we're doing something that has a "for-the-life-of-the-filesystem" performance impact? Cheers, Dave.
On 9/5/17 5:10 PM, Dave Chinner wrote: > On Tue, Sep 05, 2017 at 09:17:42AM -0500, Eric Sandeen wrote: >> >> >> On 9/5/17 1:44 AM, Dave Chinner 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! >>> >>> And, FWIW, making the warning go away if probably a bad idea, >>> because XFS only supports devices with sector sizes up >>> to 32k: >> >> Well, TBH removing this warning was my suggestion, because it's >> automatically fixing values that weren't specified by the user in >> the first place. First preference is physical sector size, then >> fallback to logical but it doesn't need to be noisy about it. >> >>> #define XFS_MIN_SECTORSIZE_LOG 9 /* i.e. 512 bytes */ >>> #define XFS_MAX_SECTORSIZE_LOG 15 /* i.e. 32768 bytes */ >>> >>> And so it should be warning about devices trying to tell it to use >>> something larger.... >> >> As long as the logical sector size is small enough, it seems like a >> silent adjustment is probably ok, no? > > Think 512e drives. Doing 512 byte sector IO is possible, but slow. > Someone might actually want to avoid that by having the filesystem > use 4k sector sizes. However, if for some reason, mkfs selects 512 > byte sectors (the logical size) rather than 4k sector size, then > shouldn't we be telling the user we're doing something that has a > "for-the-life-of-the-filesystem" performance impact? Well, sure, but it'll only select 512 if it /has/ to, i.e. if the block size is < 4k. So for the simple case of a 512e drive, our default block size is 4k, physical size is 4k, and everything is happy and fine. If the user /specifies/ a 1k block size on such a device, how much of a nanny do we really want to be about telling them this is suboptimal? There are a lot of suboptimal things you can specify on the mkfs commandline, but we don't generally choose to warn about them... -Eric > Cheers, > > Dave. > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: /** * blk_queue_physical_block_size - set physical block size for the queue * @q: the request queue for the device * @size: the physical block size, in bytes * * Description: * This should be set to the lowest possible sector size that the * hardware can operate on without reverting to read-modify-write * operations. */ Clearly for buffered loop devices, this unit is a page. The change was pedantic, so whatever, my patch above makes things backwards compatible. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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". 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.... 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. Cheers, Dave.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)) { if (bsflag) { 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? -Eric > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com> > --- > mkfs/xfs_mkfs.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index dfffae7..0bb3897 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -2011,12 +2011,6 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"), > XFS_MIN_SECTORSIZE; > > if ((blocksize < sectorsize) && (blocksize >= ft.lsectorsize)) { > - fprintf(stderr, > -_("specified blocksize %d is less than device physical sector size %d\n"), > - blocksize, ft.psectorsize); > - fprintf(stderr, > -_("switching to logical sector size %d\n"), > - ft.lsectorsize); > sectorsize = ft.lsectorsize ? ft.lsectorsize : > XFS_MIN_SECTORSIZE; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? >
On Sat, Sep 09, 2017 at 12:11:35PM +0530, Chandan Rajendra wrote: > 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. Just FYI, my mkfs series reworks this piece of code completely so there aren't any flags to check.... :P And given that we are planning on having config file overrides of default mkfs values, we need to keep this warning even on default configurations because the distro/site specific default blocksize in the config file may trigger this.... Cheers, Dave.
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index dfffae7..0bb3897 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -2011,12 +2011,6 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"), XFS_MIN_SECTORSIZE; if ((blocksize < sectorsize) && (blocksize >= ft.lsectorsize)) { - fprintf(stderr, -_("specified blocksize %d is less than device physical sector size %d\n"), - blocksize, ft.psectorsize); - fprintf(stderr, -_("switching to logical sector size %d\n"), - ft.lsectorsize); sectorsize = ft.lsectorsize ? ft.lsectorsize : XFS_MIN_SECTORSIZE; }
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. Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com> --- mkfs/xfs_mkfs.c | 6 ------ 1 file changed, 6 deletions(-)