Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 11 Dec 2020 17:35:12 -0500
From:   "Theodore Y. Ts'o" <tytso@mit.edu>
To:     Andreas Dilger <adilger@dilger.ca>
Cc:     Ext4 Developers List <linux-ext4@vger.kernel.org>,
        Saranya Muruganandam <saranyamohan@google.com>,
        Wang Shilong <wshilong@ddn.com>
Subject: Re: [PATCH RFC 4/5] ext2fs: parallel bitmap loading
Message-ID: <20201211223512.GC575698@mit.edu>
References: <20201205045856.895342-1-tytso@mit.edu>
 <20201205045856.895342-5-tytso@mit.edu>
 <4F169AE8-BFD2-4EE3-8741-7C75B8764583@dilger.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4F169AE8-BFD2-4EE3-8741-7C75B8764583@dilger.ca>
Precedence: bulk

On Thu, Dec 10, 2020 at 05:12:09PM -0700, Andreas Dilger wrote:
> > @@ -329,12 +369,20 @@ static errcode_t read_bitmaps(ext2_filsys fs, int do_inode, int do_block)
> > 				}
> > 				if (!bitmap_tail_verify((unsigned char *) block_bitmap,
> > 							block_nbytes, fs->blocksize - 1))
> > -					tail_flags |= EXT2_FLAG_BBITMAP_TAIL_PROBLEM;
> > +					*tail_flags |= EXT2_FLAG_BBITMAP_TAIL_PROBLEM;
> > 			} else
> > 				memset(block_bitmap, 0, block_nbytes);
> > 			cnt = block_nbytes << 3;
> > +#ifdef HAVE_PTHREAD
> > +			if (mutex)
> > +				pthread_mutex_lock(mutex);
> > +#endif
> > 			retval = ext2fs_set_block_bitmap_range2(fs->block_map,
> > 					       blk_itr, cnt, block_bitmap);
> > +#ifdef HAVE_PTHREAD
> > +			if (mutex)
> > +				pthread_mutex_unlock(mutex);
> > +#endif
> 
> (style) It wouldn't be terrible to have wrappers around these functions
> instead of inline #ifdef in the few places they are used, like:
> 
> #ifdef HAVE_PTHREAD
> static void unix_pthread_mutex_lock(pthread_mutex_t *mutex)
> {
> 	if (mutex)
> 		pthread_mutex_lock(mutex);
> }
> static void unix_pthread_mutex_unlock(pthread_mutex_t *mutex)
> {
> 	if (mutex)
> 		pthread_mutex_unlock(mutex);
> }
> #else
> #define unix_pthread_mutex_lock(mutex) do {} while (0)
> #define unix_pthread_mutex_unlock(mutex) do {} while (0)
> #endif

We'd also need to have a typedef for mutex_t which is either
pthreads_mutex_t if pthreads are available, or an int (or some other
placeholder type) if it isn't.

I had tried to make sure that rw_bitmaps.c will correctly compile with
HAVE_PTHREAD and HAVE_PTHREAD_H are undefined.  It looks like I didn't
quite get it completely working since there's at leasts one function
signature where we have an unprotected use of pthread_mutex_t, so
that's something we should check before finalizing the patch --- in
addition to the unprotected use of pthread_mutex_{lock,unlock} that
you pointed out.

      		    	   	 		   - Ted