From: Dave Jiang <dave.jiang@intel.com> To: Ross Zwisler <ross.zwisler@linux.intel.com> Cc: linux-nvdimm@lists.01.org, darrick.wong@oracle.com, david@fromorbit.com, linux-xfs@vger.kernel.org, linux-ext4@vger.kernel.org Subject: Re: [PATCH v3 2/3] dax: change bdev_dax_supported() to support boolean returns Date: Tue, 13 Feb 2018 13:35:19 -0700 [thread overview] Message-ID: <2e99d401-04f5-5a1c-1b7a-6c0fb6054beb@intel.com> (raw) In-Reply-To: <20180213203148.GB13682@linux.intel.com> On 02/13/2018 01:31 PM, Ross Zwisler wrote: > On Thu, Feb 08, 2018 at 05:38:19PM -0700, Dave Jiang wrote: >> The function return values are confusing with the way the function is >> named. We expect a true or false return value but it actually returns >> 0/-errno. This makes the code very confusing. Changing the return values >> to return a bool where if DAX is supported then return true and no DAX >> support returns false. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- > >> diff --git a/fs/ext2/super.c b/fs/ext2/super.c >> index 655699321c45..636b9c5e1bff 100644 >> --- a/fs/ext2/super.c >> +++ b/fs/ext2/super.c >> @@ -958,9 +958,10 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent) >> blocksize = BLOCK_SIZE << le32_to_cpu(sbi->s_es->s_log_block_size); >> >> if (sbi->s_mount_opt & EXT2_MOUNT_DAX) { >> - err = sb_dax_supported(sb, blocksize); >> - if (err) >> + if(!sb_dax_supported(sb, blocksize)) { >> + err = -EIO; > > No need to set 'err' here. This is just a temporary variable used for some > local checks later in the function. 'ret' is the value that will be returned, > and that is already initialized to -EINVAL which should be fine. Change ret to -EIO instead to set the correct error return code? > >> goto failed_mount; >> + } >> } >> >> /* If the blocksize doesn't match, re-read the thing.. */ >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >> index 804a2d6729af..7b7650ac9c53 100644 >> --- a/fs/ext4/super.c >> +++ b/fs/ext4/super.c >> @@ -3712,9 +3712,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) >> " that may contain inline data"); >> goto failed_mount; >> } >> - err = sb_dax_supported(sb, blocksize); >> - if (err) >> + if (!sb_dax_supported(sb, blocksize)) { >> + err = -EIO; > > Same comment as with the ext2 case - no need to set 'err', 'ret' is -EINVAL > and has you covered. > _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
WARNING: multiple messages have this Message-ID (diff)
From: Dave Jiang <dave.jiang@intel.com> To: Ross Zwisler <ross.zwisler@linux.intel.com> Cc: darrick.wong@oracle.com, linux-nvdimm@lists.01.org, david@fromorbit.com, linux-xfs@vger.kernel.org, linux-ext4@vger.kernel.org, dan.j.williams@intel.com Subject: Re: [PATCH v3 2/3] dax: change bdev_dax_supported() to support boolean returns Date: Tue, 13 Feb 2018 13:35:19 -0700 [thread overview] Message-ID: <2e99d401-04f5-5a1c-1b7a-6c0fb6054beb@intel.com> (raw) In-Reply-To: <20180213203148.GB13682@linux.intel.com> On 02/13/2018 01:31 PM, Ross Zwisler wrote: > On Thu, Feb 08, 2018 at 05:38:19PM -0700, Dave Jiang wrote: >> The function return values are confusing with the way the function is >> named. We expect a true or false return value but it actually returns >> 0/-errno. This makes the code very confusing. Changing the return values >> to return a bool where if DAX is supported then return true and no DAX >> support returns false. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- > >> diff --git a/fs/ext2/super.c b/fs/ext2/super.c >> index 655699321c45..636b9c5e1bff 100644 >> --- a/fs/ext2/super.c >> +++ b/fs/ext2/super.c >> @@ -958,9 +958,10 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent) >> blocksize = BLOCK_SIZE << le32_to_cpu(sbi->s_es->s_log_block_size); >> >> if (sbi->s_mount_opt & EXT2_MOUNT_DAX) { >> - err = sb_dax_supported(sb, blocksize); >> - if (err) >> + if(!sb_dax_supported(sb, blocksize)) { >> + err = -EIO; > > No need to set 'err' here. This is just a temporary variable used for some > local checks later in the function. 'ret' is the value that will be returned, > and that is already initialized to -EINVAL which should be fine. Change ret to -EIO instead to set the correct error return code? > >> goto failed_mount; >> + } >> } >> >> /* If the blocksize doesn't match, re-read the thing.. */ >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >> index 804a2d6729af..7b7650ac9c53 100644 >> --- a/fs/ext4/super.c >> +++ b/fs/ext4/super.c >> @@ -3712,9 +3712,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) >> " that may contain inline data"); >> goto failed_mount; >> } >> - err = sb_dax_supported(sb, blocksize); >> - if (err) >> + if (!sb_dax_supported(sb, blocksize)) { >> + err = -EIO; > > Same comment as with the ext2 case - no need to set 'err', 'ret' is -EINVAL > and has you covered. >
next prev parent reply other threads:[~2018-02-13 20:29 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-09 0:38 [PATCH v3 0/3] minimal DAX support for XFS realtime device Dave Jiang 2018-02-09 0:38 ` Dave Jiang 2018-02-09 0:38 ` [PATCH v3 1/3] fs: allow per-device dax status checking for filesystems Dave Jiang 2018-02-09 0:38 ` Dave Jiang 2018-02-09 0:38 ` [PATCH v3 2/3] dax: change bdev_dax_supported() to support boolean returns Dave Jiang 2018-02-09 0:38 ` Dave Jiang 2018-02-09 0:38 ` Dave Jiang 2018-02-13 20:31 ` Ross Zwisler 2018-02-13 20:31 ` Ross Zwisler 2018-02-13 20:31 ` Ross Zwisler 2018-02-13 20:35 ` Dave Jiang [this message] 2018-02-13 20:35 ` Dave Jiang 2018-02-13 20:43 ` Ross Zwisler 2018-02-13 20:43 ` Ross Zwisler 2018-02-09 0:38 ` [PATCH v3 3/3] xfs: reject removal of realtime flag when datadev doesn't support DAX Dave Jiang 2018-02-09 0:38 ` Dave Jiang 2018-02-09 0:38 ` Dave Jiang
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=2e99d401-04f5-5a1c-1b7a-6c0fb6054beb@intel.com \ --to=dave.jiang@intel.com \ --cc=darrick.wong@oracle.com \ --cc=david@fromorbit.com \ --cc=linux-ext4@vger.kernel.org \ --cc=linux-nvdimm@lists.01.org \ --cc=linux-xfs@vger.kernel.org \ --cc=ross.zwisler@linux.intel.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.