Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1202319 > unrolled thread

[PATCH] fs: inode_set_flags() replace opencoded set_mask_bits()

Back to article view | Back to linux.kernel

Contents

  [PATCH] fs: inode_set_flags() replace opencoded set_mask_bits() Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2015-08-07 09:00 +0200
    Re: [PATCH] fs: inode_set_flags() replace opencoded set_mask_bits() Peter Zijlstra <peterz@infradead.org> - 2015-08-07 13:20 +0200
      Re: [PATCH] fs: inode_set_flags() replace opencoded set_mask_bits() Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2015-08-07 13:50 +0200

#1202319 — [PATCH] fs: inode_set_flags() replace opencoded set_mask_bits()

It seems that 5f16f3225b0624 and 00a1a053ebe5, both with same commitlog
("ext4: atomically set inode->i_flags in ext4_set_inode_flags()")
introduced the set_mask_bits API, but somehow missed not using it in
ext4 in the end

Also, set_mask_bits is used in fs quite a bit and we can possibly come up
with a generic llsc based implementation (w/o the cmpxchg loop)

Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 fs/inode.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index d30640f7a193..d892ff711615 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2005,13 +2005,7 @@ EXPORT_SYMBOL(inode_dio_wait);
 void inode_set_flags(struct inode *inode, unsigned int flags,
 		     unsigned int mask)
 {
-	unsigned int old_flags, new_flags;
-
 	WARN_ON_ONCE(flags & ~mask);
-	do {
-		old_flags = ACCESS_ONCE(inode->i_flags);
-		new_flags = (old_flags & ~mask) | flags;
-	} while (unlikely(cmpxchg(&inode->i_flags, old_flags,
-				  new_flags) != old_flags));
+	set_mask_bits(&inode->i_flags, mask, flags);
 }
 EXPORT_SYMBOL(inode_set_flags);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1202596

On Fri, Aug 07, 2015 at 12:25:04PM +0530, Vineet Gupta wrote:
> It seems that 5f16f3225b0624 and 00a1a053ebe5, both with same commitlog
> ("ext4: atomically set inode->i_flags in ext4_set_inode_flags()")
> introduced the set_mask_bits API, but somehow missed not using it in
> ext4 in the end
> 
> Also, set_mask_bits is used in fs quite a bit and we can possibly come up
> with a generic llsc based implementation (w/o the cmpxchg loop)

May I also suggest changing the return value of set_mask_bits() to old.

You can compute the new value given old, but you cannot compute the old
value given new, therefore old is the better return value. Also, no
current user seems to use the return value, so changing it is without
risk.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202607

On Friday 07 August 2015 04:40 PM, Peter Zijlstra wrote:
> On Fri, Aug 07, 2015 at 12:25:04PM +0530, Vineet Gupta wrote:
>> > It seems that 5f16f3225b0624 and 00a1a053ebe5, both with same commitlog
>> > ("ext4: atomically set inode->i_flags in ext4_set_inode_flags()")
>> > introduced the set_mask_bits API, but somehow missed not using it in
>> > ext4 in the end
>> > 
>> > Also, set_mask_bits is used in fs quite a bit and we can possibly come up
>> > with a generic llsc based implementation (w/o the cmpxchg loop)
> May I also suggest changing the return value of set_mask_bits() to old.
>
> You can compute the new value given old, but you cannot compute the old
> value given new, therefore old is the better return value. Also, no
> current user seems to use the return value, so changing it is without
> risk.

Makes sense - will do that early next week !
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web