[Ocfs2-devel] [PATCH] remove ocfs2_write_blocks

[Ocfs2-devel] [PATCH] remove ocfs2_write_blocks

[Christoph Hellwig]

we only need ocfs2_write_block because there's never more than one
buffer written.

Also remove lots of useless checks and replace the useful ones with
BUG_ON.

While we're at it, can someone please audit the buffer_head end_io
handlers in ocfs?  comparing them to the generic ones in fs/buffer.c
the buffered I/O in ocfs seems to lack grabbing a reference for the
duration of the I/O.  It might in fact be possible to just use the
generic completetion handlers.


Index: fs/ocfs2/buffer_head_io.c
===================================================================
--- fs/ocfs2/buffer_head_io.c	(revision 2593)
+++ fs/ocfs2/buffer_head_io.c	(working copy)
@@ -52,21 +52,16 @@
 	unlock_buffer(bh);
 }
 
-int ocfs2_write_blocks(ocfs2_super *osb, struct buffer_head *bhs[],
-		       int nr, struct inode *inode)
+int ocfs2_write_block(ocfs2_super *osb, struct buffer_head *bh,
+				    struct inode *inode)
 {
 	int status = 0;
-	int i;
-	struct buffer_head *bh;
 
-	mlog_entry("(bh[0]->b_blocknr = %llu, nr=%d, inode=%p)\n",
-		   (unsigned long long)bhs[0]->b_blocknr, nr, inode);
+	mlog_entry("(bh->b_blocknr = %llu, inode=%p)\n",
+		   (unsigned long long)bh->b_blocknr, inode);
 
-	if (osb == NULL || osb->sb == NULL || bhs == NULL) {
-		status = -EINVAL;
-		mlog_errno(status);
-		goto bail;
-	}
+	BUG_ON(bh->b_blocknr < OCFS2_SUPER_BLOCK_BLKNO);
+	BUG_ON(buffer_jbd(bh));
 
 	/* No need to check for a soft readonly file system here. non
 	 * journalled writes are only ever done on system files which
@@ -76,70 +71,34 @@
 		goto bail;
 	}
 
-	if (inode)
-		down(&OCFS2_I(inode)->ip_io_sem);
-	for (i = 0 ; i < nr ; i++) {
-		bh = bhs[i];
-		if (bh == NULL) {
-			if (inode)
-				up(&OCFS2_I(inode)->ip_io_sem);
-			status = -EIO;
-			mlog_errno(status);
-			goto bail;
-		}
+	down(&OCFS2_I(inode)->ip_io_sem);
 
-		if (unlikely(bh->b_blocknr < OCFS2_SUPER_BLOCK_BLKNO)) {
-			BUG();
-			status = -EIO;
-			mlog_errno(status);
-			goto bail;
-		}
+	lock_buffer(bh);
+	set_buffer_uptodate(bh);
 
-		if (unlikely(buffer_jbd(bh))) {
-			/* What are you thinking?! */
-			mlog(ML_ERROR, "trying to write a jbd managed bh "
-				       "(blocknr = %llu), nr=%d\n",
-			     (unsigned long long)bh->b_blocknr, nr);
-			BUG();
-		}
+	/* remove from dirty list before I/O. */
+	clear_buffer_dirty(bh);
 
-		lock_buffer(bh);
+	bh->b_end_io = ocfs2_end_buffer_io_sync;
+	submit_bh(WRITE, bh);
 
-		set_buffer_uptodate(bh);
+	wait_on_buffer(bh);
 
-		/* remove from dirty list before I/O. */
-		clear_buffer_dirty(bh);
-
-		bh->b_end_io = ocfs2_end_buffer_io_sync;
-		submit_bh(WRITE, bh);
+	if (buffer_uptodate(bh))
+		ocfs2_set_buffer_uptodate(inode, bh);
+	else {
+		/* Status won't be cleared from here on out,
+		 * so we can safely record this and loop back
+		 * to cleanup the other buffers. Don't need to
+		 * remove the clustered uptodate information
+		 * for this bh as it's not marked locally
+		 * uptodate. */
+		status = -EIO;
+		brelse(bh);
 	}
 
-	for (i = (nr - 1) ; i >= 0; i--) {
-		bh = bhs[i];
-
-		wait_on_buffer(bh);
-
-		if (!buffer_uptodate(bh)) {
-			/* Status won't be cleared from here on out,
-			 * so we can safely record this and loop back
-			 * to cleanup the other buffers. Don't need to
-			 * remove the clustered uptodate information
-			 * for this bh as it's not marked locally
-			 * uptodate. */
-			status = -EIO;
-			brelse(bh);
-			bhs[i] = NULL;
-			continue;
-		}
-
-		if (inode)
-			ocfs2_set_buffer_uptodate(inode, bh);
-	}
-	if (inode)
-		up(&OCFS2_I(inode)->ip_io_sem);
-
-bail:
-
+	up(&OCFS2_I(inode)->ip_io_sem);
+ bail:
 	mlog_exit(status);
 	return status;
 }
Index: fs/ocfs2/buffer_head_io.h
===================================================================
--- fs/ocfs2/buffer_head_io.h	(revision 2593)
+++ fs/ocfs2/buffer_head_io.h	(working copy)
@@ -32,19 +32,13 @@
 			     int uptodate);
 
 /* Yosh made me do it. */
-static inline int ocfs2_write_block(ocfs2_super         *osb,
-				    struct buffer_head  *bh,
-				    struct inode        *inode);
 static inline int ocfs2_read_block(ocfs2_super          *osb,
 				   u64                  off,
 				   struct buffer_head **bh,
 				   int                  flags,
 				   struct inode        *inode);
 
-int ocfs2_write_blocks(ocfs2_super          *osb,
-		       struct buffer_head  *bh[],
-		       int                  nr,
-		       struct inode        *inode);
+int ocfs2_write_block(ocfs2_super *, struct buffer_head *, struct inode *);
 int ocfs2_read_blocks(ocfs2_super          *osb,
 		      u64                  block,
 		      int                  nr,
@@ -56,16 +50,6 @@
 #define OCFS2_BH_CACHED            1
 #define OCFS2_BH_READAHEAD         8	/* use this to pass READA down to submit_bh */
 
-static inline int ocfs2_write_block(ocfs2_super * osb, struct buffer_head *bh,
-				    struct inode *inode)
-{
-	int status;
-
-	status = ocfs2_write_blocks(osb, &bh, 1, inode);
-
-	return status;
-}
-
 static inline int ocfs2_read_block(ocfs2_super * osb, u64 off,
 				   struct buffer_head **bh, int flags,
 				   struct inode *inode)

[Ocfs2-devel] [PATCH] remove ocfs2_write_blocks

[Mark Fasheh]

Thanks - committed.

I took some liberties with the patch:
- merged it with the newest version of buffer_head_io.c (it got some
  readonly goodness since your patch was created)
- added a couple minor cleanups of my own
- you forgot to update a comment, which I did
	 --Mark

On Sun, Sep 18, 2005 at 12:36:17PM +0200, Christoph Hellwig wrote:
> we only need ocfs2_write_block because there's never more than one
> buffer written.
> 
> Also remove lots of useless checks and replace the useful ones with
> BUG_ON.
> 
> While we're at it, can someone please audit the buffer_head end_io
> handlers in ocfs?  comparing them to the generic ones in fs/buffer.c
> the buffered I/O in ocfs seems to lack grabbing a reference for the
> duration of the I/O.  It might in fact be possible to just use the
> generic completetion handlers.
> 
> 
> Index: fs/ocfs2/buffer_head_io.c
> ===================================================================
> --- fs/ocfs2/buffer_head_io.c	(revision 2593)
> +++ fs/ocfs2/buffer_head_io.c	(working copy)
> @@ -52,21 +52,16 @@
>  	unlock_buffer(bh);
>  }
>  
> -int ocfs2_write_blocks(ocfs2_super *osb, struct buffer_head *bhs[],
> -		       int nr, struct inode *inode)
> +int ocfs2_write_block(ocfs2_super *osb, struct buffer_head *bh,
> +				    struct inode *inode)
>  {
>  	int status = 0;
> -	int i;
> -	struct buffer_head *bh;
>  
> -	mlog_entry("(bh[0]->b_blocknr = %llu, nr=%d, inode=%p)\n",
> -		   (unsigned long long)bhs[0]->b_blocknr, nr, inode);
> +	mlog_entry("(bh->b_blocknr = %llu, inode=%p)\n",
> +		   (unsigned long long)bh->b_blocknr, inode);
>  
> -	if (osb == NULL || osb->sb == NULL || bhs == NULL) {
> -		status = -EINVAL;
> -		mlog_errno(status);
> -		goto bail;
> -	}
> +	BUG_ON(bh->b_blocknr < OCFS2_SUPER_BLOCK_BLKNO);
> +	BUG_ON(buffer_jbd(bh));
>  
>  	/* No need to check for a soft readonly file system here. non
>  	 * journalled writes are only ever done on system files which
> @@ -76,70 +71,34 @@
>  		goto bail;
>  	}
>  
> -	if (inode)
> -		down(&OCFS2_I(inode)->ip_io_sem);
> -	for (i = 0 ; i < nr ; i++) {
> -		bh = bhs[i];
> -		if (bh == NULL) {
> -			if (inode)
> -				up(&OCFS2_I(inode)->ip_io_sem);
> -			status = -EIO;
> -			mlog_errno(status);
> -			goto bail;
> -		}
> +	down(&OCFS2_I(inode)->ip_io_sem);
>  
> -		if (unlikely(bh->b_blocknr < OCFS2_SUPER_BLOCK_BLKNO)) {
> -			BUG();
> -			status = -EIO;
> -			mlog_errno(status);
> -			goto bail;
> -		}
> +	lock_buffer(bh);
> +	set_buffer_uptodate(bh);
>  
> -		if (unlikely(buffer_jbd(bh))) {
> -			/* What are you thinking?! */
> -			mlog(ML_ERROR, "trying to write a jbd managed bh "
> -				       "(blocknr = %llu), nr=%d\n",
> -			     (unsigned long long)bh->b_blocknr, nr);
> -			BUG();
> -		}
> +	/* remove from dirty list before I/O. */
> +	clear_buffer_dirty(bh);
>  
> -		lock_buffer(bh);
> +	bh->b_end_io = ocfs2_end_buffer_io_sync;
> +	submit_bh(WRITE, bh);
>  
> -		set_buffer_uptodate(bh);
> +	wait_on_buffer(bh);
>  
> -		/* remove from dirty list before I/O. */
> -		clear_buffer_dirty(bh);
> -
> -		bh->b_end_io = ocfs2_end_buffer_io_sync;
> -		submit_bh(WRITE, bh);
> +	if (buffer_uptodate(bh))
> +		ocfs2_set_buffer_uptodate(inode, bh);
> +	else {
> +		/* Status won't be cleared from here on out,
> +		 * so we can safely record this and loop back
> +		 * to cleanup the other buffers. Don't need to
> +		 * remove the clustered uptodate information
> +		 * for this bh as it's not marked locally
> +		 * uptodate. */
> +		status = -EIO;
> +		brelse(bh);
>  	}
>  
> -	for (i = (nr - 1) ; i >= 0; i--) {
> -		bh = bhs[i];
> -
> -		wait_on_buffer(bh);
> -
> -		if (!buffer_uptodate(bh)) {
> -			/* Status won't be cleared from here on out,
> -			 * so we can safely record this and loop back
> -			 * to cleanup the other buffers. Don't need to
> -			 * remove the clustered uptodate information
> -			 * for this bh as it's not marked locally
> -			 * uptodate. */
> -			status = -EIO;
> -			brelse(bh);
> -			bhs[i] = NULL;
> -			continue;
> -		}
> -
> -		if (inode)
> -			ocfs2_set_buffer_uptodate(inode, bh);
> -	}
> -	if (inode)
> -		up(&OCFS2_I(inode)->ip_io_sem);
> -
> -bail:
> -
> +	up(&OCFS2_I(inode)->ip_io_sem);
> + bail:
>  	mlog_exit(status);
>  	return status;
>  }
> Index: fs/ocfs2/buffer_head_io.h
> ===================================================================
> --- fs/ocfs2/buffer_head_io.h	(revision 2593)
> +++ fs/ocfs2/buffer_head_io.h	(working copy)
> @@ -32,19 +32,13 @@
>  			     int uptodate);
>  
>  /* Yosh made me do it. */
> -static inline int ocfs2_write_block(ocfs2_super         *osb,
> -				    struct buffer_head  *bh,
> -				    struct inode        *inode);
>  static inline int ocfs2_read_block(ocfs2_super          *osb,
>  				   u64                  off,
>  				   struct buffer_head **bh,
>  				   int                  flags,
>  				   struct inode        *inode);
>  
> -int ocfs2_write_blocks(ocfs2_super          *osb,
> -		       struct buffer_head  *bh[],
> -		       int                  nr,
> -		       struct inode        *inode);
> +int ocfs2_write_block(ocfs2_super *, struct buffer_head *, struct inode *);
>  int ocfs2_read_blocks(ocfs2_super          *osb,
>  		      u64                  block,
>  		      int                  nr,
> @@ -56,16 +50,6 @@
>  #define OCFS2_BH_CACHED            1
>  #define OCFS2_BH_READAHEAD         8	/* use this to pass READA down to submit_bh */
>  
> -static inline int ocfs2_write_block(ocfs2_super * osb, struct buffer_head *bh,
> -				    struct inode *inode)
> -{
> -	int status;
> -
> -	status = ocfs2_write_blocks(osb, &bh, 1, inode);
> -
> -	return status;
> -}
> -
>  static inline int ocfs2_read_block(ocfs2_super * osb, u64 off,
>  				   struct buffer_head **bh, int flags,
>  				   struct inode *inode)
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel
--
Mark Fasheh
Senior Software Developer, Oracle
mark.fasheh@oracle.com

[Ocfs2-devel] [PATCH] remove ocfs2_write_blocks

[Christoph Hellwig]

On Tue, Sep 20, 2005 at 01:24:39PM -0700, Mark Fasheh wrote:
> Thanks - committed.
> 
> I took some liberties with the patch:
> - merged it with the newest version of buffer_head_io.c (it got some
>   readonly goodness since your patch was created)
> - added a couple minor cleanups of my own
> - you forgot to update a comment, which I did

I think you lost the get_bh() that zab added after I submitted the
patch.

[Ocfs2-devel] [PATCH] remove ocfs2_write_blocks

[Mark Fasheh]

On Tue, Sep 20, 2005 at 10:27:19PM +0200, Christoph Hellwig wrote:
> I think you lost the get_bh() that zab added after I submitted the
> patch.
Erf, you're right. Thanks for the catch...
	--Mark

--
Mark Fasheh
Senior Software Developer, Oracle
mark.fasheh@oracle.com