lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190731144639.GG15806@quack2.suse.cz>
Date:   Wed, 31 Jul 2019 16:46:39 +0200
From:   Jan Kara <jack@...e.cz>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Matthew Wilcox <willy@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Anna-Maria Gleixner <anna-maria@...utronix.de>,
        Sebastian Siewior <bigeasy@...utronix.de>,
        Theodore Tso <tytso@....edu>, Julia Cartwright <julia@...com>,
        Jan Kara <jack@...e.com>, linux-ext4@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: [patch 2/4] fs/buffer: Move BH_Uptodate_Lock locking into
 wrapper functions

On Tue 30-07-19 13:24:54, Thomas Gleixner wrote:
> Bit spinlocks are problematic if PREEMPT_RT is enabled, because they
> disable preemption, which is undesired for latency reasons and breaks when
> regular spinlocks are taken within the bit_spinlock locked region because
> regular spinlocks are converted to 'sleeping spinlocks' on RT. So RT
> replaces the bit spinlocks with regular spinlocks to avoid this problem.
> 
> To avoid ifdeffery at the source level, wrap all BH_Uptodate_Lock bitlock
> operations with inline functions, so the spinlock substitution can be done
> at one place.
> 
> Using regular spinlocks can also be enabled for lock debugging purposes so
> the lock operations become visible to lockdep.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Cc: "Theodore Ts'o" <tytso@....edu>
> Cc: Matthew Wilcox <willy@...radead.org>
> Cc: Alexander Viro <viro@...iv.linux.org.uk>
> Cc: linux-fsdevel@...r.kernel.org

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@...e.cz>

BTW, it should be possible to get rid of BH_Uptodate_Lock altogether using
bio chaining (which was non-existent when this bh code was written) to make
sure IO completion function gets called only once all bios used to fill in
/ write out the page are done. It would be also more efficient. But I guess
that's an interesting cleanup project for some other time...

								Honza

> ---
>  fs/buffer.c                 |   20 ++++++--------------
>  fs/ext4/page-io.c           |    6 ++----
>  fs/ntfs/aops.c              |   10 +++-------
>  include/linux/buffer_head.h |   16 ++++++++++++++++
>  4 files changed, 27 insertions(+), 25 deletions(-)
> 
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -275,8 +275,7 @@ static void end_buffer_async_read(struct
>  	 * decide that the page is now completely done.
>  	 */
>  	first = page_buffers(page);
> -	local_irq_save(flags);
> -	bit_spin_lock(BH_Uptodate_Lock, &first->b_state);
> +	flags = bh_uptodate_lock_irqsave(first);
>  	clear_buffer_async_read(bh);
>  	unlock_buffer(bh);
>  	tmp = bh;
> @@ -289,8 +288,7 @@ static void end_buffer_async_read(struct
>  		}
>  		tmp = tmp->b_this_page;
>  	} while (tmp != bh);
> -	bit_spin_unlock(BH_Uptodate_Lock, &first->b_state);
> -	local_irq_restore(flags);
> +	bh_uptodate_unlock_irqrestore(first, flags);
>  
>  	/*
>  	 * If none of the buffers had errors and they are all
> @@ -302,9 +300,7 @@ static void end_buffer_async_read(struct
>  	return;
>  
>  still_busy:
> -	bit_spin_unlock(BH_Uptodate_Lock, &first->b_state);
> -	local_irq_restore(flags);
> -	return;
> +	bh_uptodate_unlock_irqrestore(first, flags);
>  }
>  
>  /*
> @@ -331,8 +327,7 @@ void end_buffer_async_write(struct buffe
>  	}
>  
>  	first = page_buffers(page);
> -	local_irq_save(flags);
> -	bit_spin_lock(BH_Uptodate_Lock, &first->b_state);
> +	flags = bh_uptodate_lock_irqsave(first);
>  
>  	clear_buffer_async_write(bh);
>  	unlock_buffer(bh);
> @@ -344,15 +339,12 @@ void end_buffer_async_write(struct buffe
>  		}
>  		tmp = tmp->b_this_page;
>  	}
> -	bit_spin_unlock(BH_Uptodate_Lock, &first->b_state);
> -	local_irq_restore(flags);
> +	bh_uptodate_unlock_irqrestore(first, flags);
>  	end_page_writeback(page);
>  	return;
>  
>  still_busy:
> -	bit_spin_unlock(BH_Uptodate_Lock, &first->b_state);
> -	local_irq_restore(flags);
> -	return;
> +	bh_uptodate_unlock_irqrestore(first, flags);
>  }
>  EXPORT_SYMBOL(end_buffer_async_write);
>  
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -90,8 +90,7 @@ static void ext4_finish_bio(struct bio *
>  		 * We check all buffers in the page under BH_Uptodate_Lock
>  		 * to avoid races with other end io clearing async_write flags
>  		 */
> -		local_irq_save(flags);
> -		bit_spin_lock(BH_Uptodate_Lock, &head->b_state);
> +		flags = bh_uptodate_lock_irqsave(head);
>  		do {
>  			if (bh_offset(bh) < bio_start ||
>  			    bh_offset(bh) + bh->b_size > bio_end) {
> @@ -103,8 +102,7 @@ static void ext4_finish_bio(struct bio *
>  			if (bio->bi_status)
>  				buffer_io_error(bh);
>  		} while ((bh = bh->b_this_page) != head);
> -		bit_spin_unlock(BH_Uptodate_Lock, &head->b_state);
> -		local_irq_restore(flags);
> +		bh_uptodate_unlock_irqrestore(head, flags);
>  		if (!under_io) {
>  			fscrypt_free_bounce_page(bounce_page);
>  			end_page_writeback(page);
> --- a/fs/ntfs/aops.c
> +++ b/fs/ntfs/aops.c
> @@ -92,8 +92,7 @@ static void ntfs_end_buffer_async_read(s
>  				"0x%llx.", (unsigned long long)bh->b_blocknr);
>  	}
>  	first = page_buffers(page);
> -	local_irq_save(flags);
> -	bit_spin_lock(BH_Uptodate_Lock, &first->b_state);
> +	flags = bh_uptodate_lock_irqsave(first);
>  	clear_buffer_async_read(bh);
>  	unlock_buffer(bh);
>  	tmp = bh;
> @@ -108,8 +107,7 @@ static void ntfs_end_buffer_async_read(s
>  		}
>  		tmp = tmp->b_this_page;
>  	} while (tmp != bh);
> -	bit_spin_unlock(BH_Uptodate_Lock, &first->b_state);
> -	local_irq_restore(flags);
> +	bh_uptodate_unlock_irqrestore(first, flags);
>  	/*
>  	 * If none of the buffers had errors then we can set the page uptodate,
>  	 * but we first have to perform the post read mst fixups, if the
> @@ -142,9 +140,7 @@ static void ntfs_end_buffer_async_read(s
>  	unlock_page(page);
>  	return;
>  still_busy:
> -	bit_spin_unlock(BH_Uptodate_Lock, &first->b_state);
> -	local_irq_restore(flags);
> -	return;
> +	bh_uptodate_unlock_irqrestore(first, flags);
>  }
>  
>  /**
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -78,6 +78,22 @@ struct buffer_head {
>  	atomic_t b_count;		/* users using this buffer_head */
>  };
>  
> +static inline unsigned long bh_uptodate_lock_irqsave(struct buffer_head *bh)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	bit_spin_lock(BH_Uptodate_Lock, &bh->b_state);
> +	return flags;
> +}
> +
> +static inline void
> +bh_uptodate_unlock_irqrestore(struct buffer_head *bh, unsigned long flags)
> +{
> +	bit_spin_unlock(BH_Uptodate_Lock, &bh->b_state);
> +	local_irq_restore(flags);
> +}
> +
>  /*
>   * macro tricks to expand the set_buffer_foo(), clear_buffer_foo()
>   * and buffer_foo() functions.
> 
> 
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ