xfs
[Top] [All Lists]

Re: [PATCH V2] mkfs: handle 4k sector devices more cleanly

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH V2] mkfs: handle 4k sector devices more cleanly
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Sat, 9 Jan 2010 09:43:28 -0500
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
In-reply-to: <4B47E8C0.2040809@xxxxxxxxxxx>
References: <4B1E9A25.50108@xxxxxxxxxx> <4B476171.4020701@xxxxxxxxxxx> <20100108174400.GA17634@xxxxxxxxxxxxx> <4B47E8C0.2040809@xxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
On Fri, Jan 08, 2010 at 08:24:00PM -0600, Eric Sandeen wrote:
> Christoph Hellwig wrote:
> > On Fri, Jan 08, 2010 at 10:46:41AM -0600, Eric Sandeen wrote:
> >> +extern void platform_findsizes (char *path, int fd, long long *sz, int 
> >> *bsz);
> > 
> > Can you move the prototype from libxfs/init.h to include/libxfs.h
> > instead of adding it to the .c file?
> 
> actually it seems a little out of place in libxfs/init.h; that probably
> works but there are no other platform_* functions there...
> 
> Should this be more like an xfs_findsizes in libxfs, which
> calls platform_findsizes internally just for consistency? *shrug*

I'd just move the header around to keep it simple for now.

> The only way this is used is: it's sent to libxfs_init and then from there
> only to libxfs_open which does:
> 
>         if (!readonly && setblksize && (statb.st_mode & S_IFMT) == S_IFBLK) {
>                 if (setblksize == 1)
>                         /* use the default blocksize */
>                         (void)platform_set_blocksize(fd, path, statb.st_rdev, 
> XFS_MIN_SECTORSIZE, 0);
>                 else {
>                         /* given an explicit blocksize to use */
>                         if (platform_set_blocksize(fd, path, statb.st_rdev, 
> setblksize, 1))
>                             exit(1);
>                 }
>         }
> 
> so "1" seems to have the special meaning of "use XFS_MIN_SECTORSIZE"
> 
> Hm, ok but in my patch setblksize is 0, so it's not getting set.
> I suppose this -might- mess up other utils ...

Yes, now we do not set any size, while previously we always set
XFS_MIN_SECTORSIZE.  I don't think this actually changes anything - the
kernel doesn't make use of this blocksize information outside of
filesystems, and we already reset it on mount.

> > Maye we should just do the xi.setblksize = sectorsize unconditionally?
> 
> I think that's best.  It's already defaulted to XFS_MIN_SECTORSIZE
> so should be no functional change if it doesn't get otherwise set - although
> I think it -does- get set in all cases now - either from 
> ft.sectoralign/blocksize,
> from the explicit -s setting, or the ft.sectorsize by default.
> 
> What do you think about removing the "1" magic if so?  At that point I think
> nothing relies on it.

repair uses it in a weird way.

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