xfs
[Top] [All Lists]

Re: [PATCH] xfsprogs: make log/size consistent for mkfs's -s option

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] xfsprogs: make log/size consistent for mkfs's -s option
From: Li Zhong <zhong@xxxxxxxxxxxxxxxxxx>
Date: Sun, 29 Sep 2013 14:45:01 +0800
Cc: xfsprogs <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <5245C07A.3000700@xxxxxxxxxxx>
References: <1380272973.2836.5.camel@ThinkPad-T5421> <5245C07A.3000700@xxxxxxxxxxx>
On Fri, 2013-09-27 at 12:29 -0500, Eric Sandeen wrote:
> On 9/27/13 4:09 AM, Li Zhong wrote:
> > It seems using -s log is not able to set the sectsz correctly. Because 
> > slflag
> > is set but ignored by later codes, so the advertised sector size of the 
> > device
> > is used instead.
> 
> (below is just musing about this in general, skip down to the patch for one
> flaw, I think).
> 
> One wonders why it was originally written to accept both, in the first place 
> :(
> 
> And looking at git history, checking only ssflag later was my mistake.  :(
> 
> by the time we're done with getopt, we've got both sectorsize and sectorlog
> set anyway, and we know if it was specified on the commandline.  Maybe we 
> should
> just set them both right after getopt, like:
> 
>         /*
>          * Later code wants to know if the user manually set a value.
>          * There are two ways to specify on the cmdline; as size or as a log.
>          * if either was used, set both flags - from here on it simply means 
>          * "manually set"
>          */
> 
>         if (ssflag || slflag)
>                 ssflag = slflag = 1;
>         <etc for other flags>

I think it is better, after this is done, later code could use one
*sflag to check whether the value is manually set. I will give it a try.

> 
> Anyway, other than one problem below, I think this is ok to solve this
> particular problem.  The others, at least nsflag/nlflag, isflag/ilflag,
> and bsflag/blflag all look ok.
> 
> I just wonder if we need to re-think how this is handled in general,
> so for all of the various (size=|log=) type options, we don't have to
> keep remembering to check both flags.
> 
> (mkfs.xfs is so crufty :(  )
> 
> > $ mkfs.xfs -f -s size=4096 /dev/sdd
> > meta-data=/dev/sdd               isize=256    agcount=2, agsize=4096 blks
> >          =                       sectsz=4096  attr=2, projid32bit=1
> >      ......
> > 
> > $ mkfs.xfs -f -s log=12 /dev/sdd
> > meta-data=/dev/sdd               isize=256    agcount=2, agsize=4096 blks
> >          =                       sectsz=2048  attr=2, projid32bit=1
> >      ......
> > 
> > Signed-off-by: Li Zhong <zhong@xxxxxxxxxxxxxxxxxx>
> > ---
> >  mkfs/xfs_mkfs.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index eafbed3..9243044 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -1693,7 +1693,7 @@ main(
> >              * ft.sectoralign will never be set.
> >              */
> >             sectorsize = blocksize;
> > -   } else if (!ssflag) {
> > +   } else if (!ssflag && !slflag) {
> >             /*
> >              * Unless specified manually on the command line use the
> >              * advertised sector size of the device.  We use the physical
> > @@ -1721,7 +1721,7 @@ _("switching to logical sector size %d\n"),
> >             }
> >     }
> >  
> > -   if (ft.sectoralign || !ssflag) {
> > +   if (ft.sectoralign || !ssflag || !slflag) {
> 
> Shouldn't this be:
> 
> if (ft.sectoralign || (!ssflag && !slflag)) {
> 
> ?  Because today only one or the other can be set;  !ssflag || !slflag will 
> always be true I think.

Sorry, it's my mistake... I'll update it and give a v2. 

Thanks, Zhong

> 
> Thanks,  
> -Eric
> 
> >             sectorlog = libxfs_highbit32(sectorsize);
> >             if (loginternal) {
> >                     lsectorsize = sectorsize;
> > @@ -1731,7 +1731,7 @@ _("switching to logical sector size %d\n"),
> >  
> >     if (sectorsize < XFS_MIN_SECTORSIZE ||
> >         sectorsize > XFS_MAX_SECTORSIZE || sectorsize > blocksize) {
> > -           if (ssflag)
> > +           if (ssflag || slflag)
> >                     fprintf(stderr, _("illegal sector size %d\n"), 
> > sectorsize);
> >             else
> >                     fprintf(stderr,
> > 
> 


<Prev in Thread] Current Thread [Next in Thread>