From: Dave Chinner <david@fromorbit.com>
To: Ben Myers <bpm@sgi.com>
Cc: linux-fsdevel@vger.kernel.org, olaf@sgi.com, xfs@oss.sgi.com
Subject: Re: [PATCH 10/16] xfs: store utf8version in the superblock
Date: Tue, 7 Oct 2014 08:53:52 +1100 [thread overview]
Message-ID: <20141006215352.GG2301@dastard> (raw)
In-Reply-To: <20141003220031.GI1865@sgi.com>
On Fri, Oct 03, 2014 at 05:00:31PM -0500, Ben Myers wrote:
> From: Ben Myers <bpm@sgi.com>
>
> The utf8 version a filesystem was created with needs to be stored in
> order that normalizations will remain stable over the lifetime of the
> filesystem. Convert sb_pad to sb_utf8version in the super block. This
> also adds checks at mount time to see whether the unicode normalization
> module has support for the version of unicode that the filesystem
> requires. If not we fail the mount.
>
> Signed-off-by: Ben Myers <bpm@sgi.com>
> ---
> fs/xfs/libxfs/xfs_dir2.c | 28 ++++++++++++++++---
> fs/xfs/libxfs/xfs_sb.c | 4 +--
> fs/xfs/libxfs/xfs_sb.h | 10 ++++---
> fs/xfs/libxfs/xfs_utf8.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
> fs/xfs/libxfs/xfs_utf8.h | 24 +++++++++++++++++
> 5 files changed, 126 insertions(+), 10 deletions(-)
> create mode 100644 fs/xfs/libxfs/xfs_utf8.c
> create mode 100644 fs/xfs/libxfs/xfs_utf8.h
>
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index 4eb0973..2c89211 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -157,10 +157,30 @@ xfs_da_mount(
> (uint)sizeof(xfs_da_node_entry_t);
> dageo->magicpct = (dageo->blksize * 37) / 100;
>
> - if (xfs_sb_version_hasasciici(&mp->m_sb))
> - mp->m_dirnameops = &xfs_ascii_ci_nameops;
> - else
> - mp->m_dirnameops = &xfs_default_nameops;
> + if (xfs_sb_version_hasutf8(&mp->m_sb)) {
> +#ifdef CONFIG_XFS_UTF8
> + if (!xfs_utf8_version_ok(mp))
> + return -ENOSYS;
> +
> + /* XXX these are replaced in the next patch need
> + to do some kind of reordering here */
> + if (xfs_sb_version_hasasciici(&mp->m_sb))
> + mp->m_dirnameops = &xfs_ascii_ci_nameops;
> + else
> + mp->m_dirnameops = &xfs_default_nameops;
> +#else
> + xfs_warn(mp,
> +"Recompile XFS with CONFIG_XFS_UTF8 to mount this filesystem");
> + kmem_free(mp->m_dir_geo);
> + kmem_free(mp->m_attr_geo);
> + return -ENOSYS;
> +#endif
This config check doesn't belong here. Validation of superblock
fields vs the current config goes in the superblock verifier. I also
think that indication of UTF8 support being compiled in needs to go
in the XFS_VERSION_STRING, not have ifdef hackery added into the
code.
i.e. the mount should fail very early on with a superblock
verification failure from xfs_mount_validate_sb().
> + } else {
> + if (xfs_sb_version_hasasciici(&mp->m_sb))
> + mp->m_dirnameops = &xfs_ascii_ci_nameops;
> + else
> + mp->m_dirnameops = &xfs_default_nameops;
> + }
>
> return 0;
> }
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index ad525a5..1ee7d33 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -99,7 +99,7 @@ static const struct {
> { offsetof(xfs_sb_t, sb_features_incompat), 0 },
> { offsetof(xfs_sb_t, sb_features_log_incompat), 0 },
> { offsetof(xfs_sb_t, sb_crc), 0 },
> - { offsetof(xfs_sb_t, sb_pad), 0 },
> + { offsetof(xfs_sb_t, sb_utf8version), 0 },
> { offsetof(xfs_sb_t, sb_pquotino), 0 },
> { offsetof(xfs_sb_t, sb_lsn), 0 },
> { sizeof(xfs_sb_t), 0 }
> @@ -443,7 +443,7 @@ __xfs_sb_from_disk(
> to->sb_features_incompat = be32_to_cpu(from->sb_features_incompat);
> to->sb_features_log_incompat =
> be32_to_cpu(from->sb_features_log_incompat);
> - to->sb_pad = 0;
> + to->sb_utf8version = be32_to_cpu(from->sb_utf8version);
> to->sb_pquotino = be64_to_cpu(from->sb_pquotino);
> to->sb_lsn = be64_to_cpu(from->sb_lsn);
> /* Convert on-disk flags to in-memory flags? */
> diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
> index 525eacb..dc7b6c6 100644
> --- a/fs/xfs/libxfs/xfs_sb.h
> +++ b/fs/xfs/libxfs/xfs_sb.h
> @@ -159,7 +159,7 @@ typedef struct xfs_sb {
> __uint32_t sb_features_log_incompat;
>
> __uint32_t sb_crc; /* superblock crc */
> - __uint32_t sb_pad;
> + __uint32_t sb_utf8version; /* unicode version */
>
> xfs_ino_t sb_pquotino; /* project quota inode */
> xfs_lsn_t sb_lsn; /* last write sequence */
> @@ -245,7 +245,7 @@ typedef struct xfs_dsb {
> __be32 sb_features_log_incompat;
>
> __le32 sb_crc; /* superblock crc */
> - __be32 sb_pad;
> + __be32 sb_utf8version; /* version of unicode */
>
> __be64 sb_pquotino; /* project quota inode */
> __be64 sb_lsn; /* last write sequence */
> @@ -271,7 +271,7 @@ typedef enum {
> XFS_SBS_LOGSECTLOG, XFS_SBS_LOGSECTSIZE, XFS_SBS_LOGSUNIT,
> XFS_SBS_FEATURES2, XFS_SBS_BAD_FEATURES2, XFS_SBS_FEATURES_COMPAT,
> XFS_SBS_FEATURES_RO_COMPAT, XFS_SBS_FEATURES_INCOMPAT,
> - XFS_SBS_FEATURES_LOG_INCOMPAT, XFS_SBS_CRC, XFS_SBS_PAD,
> + XFS_SBS_FEATURES_LOG_INCOMPAT, XFS_SBS_CRC, XFS_SBS_UTF8VERSION,
> XFS_SBS_PQUOTINO, XFS_SBS_LSN,
> XFS_SBS_FIELDCOUNT
> } xfs_sb_field_t;
> @@ -303,6 +303,7 @@ typedef enum {
> #define XFS_SB_FEATURES_INCOMPAT XFS_SB_MVAL(FEATURES_INCOMPAT)
> #define XFS_SB_FEATURES_LOG_INCOMPAT XFS_SB_MVAL(FEATURES_LOG_INCOMPAT)
> #define XFS_SB_CRC XFS_SB_MVAL(CRC)
> +#define XFS_SB_UTF8VERSION XFS_SB_MVAL(UTF8VERSION)
> #define XFS_SB_PQUOTINO XFS_SB_MVAL(PQUOTINO)
> #define XFS_SB_NUM_BITS ((int)XFS_SBS_FIELDCOUNT)
> #define XFS_SB_ALL_BITS ((1LL << XFS_SB_NUM_BITS) - 1)
> @@ -313,7 +314,8 @@ typedef enum {
> XFS_SB_ICOUNT | XFS_SB_IFREE | XFS_SB_FDBLOCKS | XFS_SB_FEATURES2 | \
> XFS_SB_BAD_FEATURES2 | XFS_SB_FEATURES_COMPAT | \
> XFS_SB_FEATURES_RO_COMPAT | XFS_SB_FEATURES_INCOMPAT | \
> - XFS_SB_FEATURES_LOG_INCOMPAT | XFS_SB_PQUOTINO)
> + XFS_SB_FEATURES_LOG_INCOMPAT | XFS_SB_UTF8VERSION | \
> + XFS_SB_PQUOTINO)
We should never be modifying the utf8 version number from the
kernel, so this shouldn't be set in the XFS_SB_MOD_BITS mask.
> diff --git a/fs/xfs/libxfs/xfs_utf8.c b/fs/xfs/libxfs/xfs_utf8.c
> new file mode 100644
> index 0000000..7e63111
> --- /dev/null
> +++ b/fs/xfs/libxfs/xfs_utf8.c
> @@ -0,0 +1,70 @@
> +/*
> + * Copyright (c) 2014 SGI.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write the Free Software Foundation,
> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "xfs.h"
> +#include "xfs_fs.h"
> +#include "xfs_types.h"
> +#include "xfs_bit.h"
> +#include "xfs_log_format.h"
> +#include "xfs_inum.h"
> +#include "xfs_trans.h"
> +#include "xfs_trans_resv.h"
> +#include "xfs_sb.h"
> +#include "xfs_ag.h"
> +#include "xfs_da_format.h"
> +#include "xfs_da_btree.h"
> +#include "xfs_dir2.h"
> +#include "xfs_mount.h"
> +#include "xfs_da_btree.h"
> +#include "xfs_format.h"
> +#include "xfs_bmap_btree.h"
> +#include "xfs_alloc_btree.h"
> +#include "xfs_dinode.h"
> +#include "xfs_inode.h"
> +#include "xfs_inode_item.h"
> +#include "xfs_bmap.h"
> +#include "xfs_error.h"
> +#include "xfs_trace.h"
> +#include "xfs_utf8.h"
This may sound pedantic, but in all the libxfs rework I've managed
to standadise the initial include file order to be roughly:
#include "xfs.h"
#include "xfs_fs.h"
#include "xfs_shared.h"
#include "xfs_format.h"
#include "xfs_log_format.h"
#include "xfs_trans_resv.h"
#include "xfs_bit.h"
#include "xfs_inum.h"
#include "xfs_sb.h"
#include "xfs_ag.h"
#include "xfs_mount.h"
#include "xfs_da_format.h"
i.e. include all the type, shared and on-disk format information
first. Can you re-order these to follow the same convention?
> +#include <linux/utf8norm.h>
And that should end up being included from fs/xfs/xfs_linux.h,
because we can't include things directly from the linux kernel
headers in fs/xfs/libxfs/ files.
> +
> +int
Bool.
> +xfs_utf8_version_ok(
> + struct xfs_mount *mp)
> +{
> + int major, minor, revision;
> +
> + if (utf8version_is_supported(mp->m_sb.sb_utf8version))
> + return 1;
return true;
> +
> + major = mp->m_sb.sb_utf8version >> UNICODE_MAJ_SHIFT;
> + minor = (mp->m_sb.sb_utf8version & 0xff00) >> UNICODE_MIN_SHIFT;
> + revision = mp->m_sb.sb_utf8version & 0xff;
> +
> + if (revision) {
> + xfs_warn(mp,
> + "Unicode version %d.%d.%d not supported by utf8norm.ko",
> + major, minor, revision);
> + } else {
> + xfs_warn(mp,
> + "Unicode version %d.%d not supported by utf8norm.ko",
> + major, minor);
> + }
why do you need two different print statements? Version X.Y.0 is
pretty much recognisable as being the same as X.Y....
> +
> + return 0;
return false;
> +}
> diff --git a/fs/xfs/libxfs/xfs_utf8.h b/fs/xfs/libxfs/xfs_utf8.h
> new file mode 100644
> index 0000000..8a700de
> --- /dev/null
> +++ b/fs/xfs/libxfs/xfs_utf8.h
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (c) 2014 SGI.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write the Free Software Foundation,
> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef XFS_UTF8_H
> +#define XFS_UTF8_H
> +
> +extern int xfs_utf8_version_ok(struct xfs_mount *);
> +
> +#endif /* XFS_UTF8_H */
Do we really need a separate header file for this?
fs/xfs/libxfs/xfs_shared.h was created for such one-off or
limited definitions that need to be shared with userspace...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2014-10-06 21:53 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-03 21:47 [RFC v3] Unicode/UTF-8 support for XFS Ben Myers
2014-10-03 21:50 ` [PATCH 01/16] lib: add unicode character database files Ben Myers
2014-10-03 21:51 ` [PATCH 02/16] scripts: add trie generator for UTF-8 Ben Myers
2014-10-03 21:54 ` [PATCH 03/16] lib: add supporting code " Ben Myers
2014-10-03 21:54 ` [PATCH 04/16] lib/utf8norm.c: reduce the size of utf8data[] Ben Myers
2014-10-05 21:52 ` Dave Chinner
2014-10-03 21:55 ` [PATCH 05/16] xfs: return the first match during case-insensitive lookup Ben Myers
2014-10-06 22:19 ` Dave Chinner
2014-10-09 15:42 ` Ben Myers
2014-10-09 20:38 ` Dave Chinner
2014-10-14 15:04 ` Ben Myers
2014-10-03 21:56 ` [PATCH 06/16] xfs: rename XFS_CMP_CASE to XFS_CMP_MATCH Ben Myers
2014-10-03 21:58 ` [PATCH 07/16] xfs: add xfs_nameops.normhash Ben Myers
2014-10-03 21:58 ` [PATCH 08/16] xfs: change interface of xfs_nameops.hashname Ben Myers
2014-10-06 22:17 ` Dave Chinner
2014-10-14 15:34 ` Ben Myers
2014-10-03 21:59 ` [PATCH 09/16] xfs: add a superblock feature bit to indicate UTF-8 support Ben Myers
2014-10-06 21:25 ` Dave Chinner
2014-10-09 15:26 ` Ben Myers
2014-10-03 22:00 ` [PATCH 10/16] xfs: store utf8version in the superblock Ben Myers
2014-10-06 21:53 ` Dave Chinner [this message]
2014-10-03 22:01 ` [PATCH 11/16] xfs: add xfs_nameops for utf8 and utf8+casefold Ben Myers
2014-10-06 22:10 ` Dave Chinner
2014-10-03 22:03 ` [PATCH 12/16] xfs: apply utf-8 normalization rules to user extended attribute names Ben Myers
2014-10-03 22:03 ` [PATCH 13/16] xfs: implement demand load of utf8norm.ko Ben Myers
2014-10-04 7:16 ` Christoph Hellwig
2014-10-09 15:19 ` Ben Myers
2014-10-03 22:04 ` [PATCH 14/16] xfs: rename XFS_IOC_FSGEOM to XFS_IOC_FSGEOM_V2 Ben Myers
2014-10-06 20:33 ` Dave Chinner
2014-10-06 20:38 ` Ben Myers
2014-10-03 22:05 ` [PATCH 15/16] xfs: xfs_fs_geometry returns a number of bytes to copy Ben Myers
2014-10-06 20:41 ` Dave Chinner
2014-10-03 22:05 ` [PATCH 16/16] xfs: add versioned fsgeom ioctl with utf8version field Ben Myers
2014-10-06 21:13 ` Dave Chinner
2014-10-03 22:06 ` [PATCH 17/35] xfsprogs: add unicode character database files Ben Myers
2014-10-03 22:07 ` [PATCH 18/35] xfsprogs: add trie generator for UTF-8 Ben Myers
2014-10-03 22:07 ` [PATCH 19/35] xfsprogs: add supporting code " Ben Myers
2014-10-03 22:08 ` [PATCH 20/35] xfsprogs: reduce the size of utf8data[] Ben Myers
2014-10-03 22:09 ` [PATCH 21/35] libxfs: return the first match during case-insensitive lookup Ben Myers
2014-10-03 22:09 ` [PATCH 22/35] libxfs: rename XFS_CMP_CASE to XFS_CMP_MATCH Ben Myers
2014-10-03 22:10 ` [PATCH 23/35] libxfs: add xfs_nameops.normhash Ben Myers
2014-10-03 22:11 ` [PATCH 24/35] libxfs: change interface of xfs_nameops.hashname Ben Myers
2014-10-03 22:11 ` [PATCH 25/35] libxfs: add a superblock feature bit to indicate UTF-8 support Ben Myers
2014-10-03 22:12 ` [PATCH 26/35] libxfs: store utf8version in the superblock Ben Myers
2014-10-03 22:13 ` [PATCH 27/35] libxfs: add xfs_nameops for utf8 and utf8+casefold Ben Myers
2014-10-03 22:13 ` [PATCH 28/35] libxfs: apply utf-8 normalization rules to user extended attribute names Ben Myers
2014-10-03 22:14 ` [PATCH 29/35] libxfs: rename XFS_IOC_FSGEOM to XFS_IOC_FSGEOM_V2 Ben Myers
2014-10-03 22:14 ` [PATCH 30/35] libxfs: add versioned fsgeom ioctl with utf8version field Ben Myers
2014-10-03 22:15 ` [PATCH 31/35] xfsprogs: add utf8 support to growfs Ben Myers
2014-10-03 22:15 ` [PATCH 32/35] xfsprogs: add utf8 support to mkfs.xfs Ben Myers
2014-10-03 22:16 ` [PATCH 33/35] xfsprogs: add utf8 support to xfs_repair Ben Myers
2014-10-03 22:16 ` [PATCH 34/35] xfsprogs: xfs_db support for sb_utf8version Ben Myers
2014-10-03 22:17 ` [PATCH 35/35] xfsprogs: add a test for utf8 support Ben Myers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20141006215352.GG2301@dastard \
--to=david@fromorbit.com \
--cc=bpm@sgi.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=olaf@sgi.com \
--cc=xfs@oss.sgi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).