xfs
[Top] [All Lists]

Re: [PATCH 1/2] vfs: split generic splice code from i_mutex locking

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] vfs: split generic splice code from i_mutex locking
From: Jan Kara <jack@xxxxxxx>
Date: Tue, 9 Aug 2011 13:36:41 +0200
Cc: xfs@xxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx
In-reply-to: <1312785927-10662-2-git-send-email-david@xxxxxxxxxxxxx>
References: <1312785927-10662-1-git-send-email-david@xxxxxxxxxxxxx> <1312785927-10662-2-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Mon 08-08-11 16:45:26, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> XFS holds locks that should be nested inside the inode->i_mutex when
> generic_file_splice_write is called. This function takes the
> i_mutex, and so we get a lock inversion that triggers lockdep
> warnings and has been found to cause real deadlocks.
> 
> XFS does not need the splice code to take the i_mutex to do the page
> cache manipulation, so modify the generic splice code to use an
> actor function for the code that currently requires the i_mutex to
> be taken.  Convert generic_file_splice_write() to use this new
> interface supplying a generic actor function that performs the same
> actions as the existing code.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
  The patch looks good to me.
Reviewed-by: Jan Kara <jack@xxxxxxx>

                                                                Honza
> ---
>  fs/splice.c            |   58 
> ++++++++++++++++++++++++++++++++++++++----------
>  include/linux/splice.h |    5 ++++
>  2 files changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/splice.c b/fs/splice.c
> index fa2defa..a02dddc 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -967,24 +967,24 @@ ssize_t splice_from_pipe(struct pipe_inode_info *pipe, 
> struct file *out,
>  }
>  
>  /**
> - * generic_file_splice_write - splice data from a pipe to a file
> + * splice_write_to_file - splice data from a pipe to a file
>   * @pipe:    pipe info
>   * @out:     file to write to
>   * @ppos:    position in @out
>   * @len:     number of bytes to splice
>   * @flags:   splice modifier flags
> + * @actor:   worker that does the splicing from the pipe to the file
>   *
>   * Description:
>   *    Will either move or copy pages (determined by @flags options) from
>   *    the given pipe inode to the given file.
>   *
>   */
> -ssize_t
> -generic_file_splice_write(struct pipe_inode_info *pipe, struct file *out,
> -                       loff_t *ppos, size_t len, unsigned int flags)
> +ssize_t splice_write_to_file(struct pipe_inode_info *pipe, struct file *out,
> +                          loff_t *ppos, size_t len, unsigned int flags,
> +                          splice_write_actor actor)
>  {
>       struct address_space *mapping = out->f_mapping;
> -     struct inode *inode = mapping->host;
>       struct splice_desc sd = {
>               .total_len = len,
>               .flags = flags,
> @@ -1001,13 +1001,8 @@ generic_file_splice_write(struct pipe_inode_info 
> *pipe, struct file *out,
>               if (ret <= 0)
>                       break;
>  
> -             mutex_lock_nested(&inode->i_mutex, I_MUTEX_CHILD);
> -             ret = file_remove_suid(out);
> -             if (!ret) {
> -                     file_update_time(out);
> -                     ret = splice_from_pipe_feed(pipe, &sd, pipe_to_file);
> -             }
> -             mutex_unlock(&inode->i_mutex);
> +             ret = actor(pipe, &sd);
> +
>       } while (ret > 0);
>       splice_from_pipe_end(pipe, &sd);
>  
> @@ -1032,7 +1027,46 @@ generic_file_splice_write(struct pipe_inode_info 
> *pipe, struct file *out,
>  
>       return ret;
>  }
> +EXPORT_SYMBOL(splice_write_to_file);
>  
> +static ssize_t generic_file_splice_write_actor(struct pipe_inode_info *pipe,
> +                                            struct splice_desc *sd)
> +{
> +     struct file *out = sd->u.file;
> +     struct inode *inode = out->f_mapping->host;
> +     ssize_t ret;
> +
> +     mutex_lock_nested(&inode->i_mutex, I_MUTEX_CHILD);
> +     ret = file_remove_suid(out);
> +     if (!ret) {
> +             file_update_time(out);
> +             ret = splice_from_pipe_feed(pipe, sd, pipe_to_file);
> +     }
> +     mutex_unlock(&inode->i_mutex);
> +
> +     return ret;
> +}
> +
> +/**
> + * generic_file_splice_write - splice data from a pipe to a file
> + * @pipe:    pipe info
> + * @out:     file to write to
> + * @ppos:    position in @out
> + * @len:     number of bytes to splice
> + * @flags:   splice modifier flags
> + *
> + * Description:
> + *    Will either move or copy pages (determined by @flags options) from
> + *    the given pipe inode to the given file.
> + *
> + */
> +ssize_t
> +generic_file_splice_write(struct pipe_inode_info *pipe, struct file *out,
> +                       loff_t *ppos, size_t len, unsigned int flags)
> +{
> +     return splice_write_to_file(pipe, out, ppos, len, flags,
> +                                 generic_file_splice_write_actor);
> +}
>  EXPORT_SYMBOL(generic_file_splice_write);
>  
>  static int write_pipe_buf(struct pipe_inode_info *pipe, struct pipe_buffer 
> *buf,
> diff --git a/include/linux/splice.h b/include/linux/splice.h
> index 26e5b61..bd14281 100644
> --- a/include/linux/splice.h
> +++ b/include/linux/splice.h
> @@ -61,6 +61,8 @@ typedef int (splice_actor)(struct pipe_inode_info *, struct 
> pipe_buffer *,
>                          struct splice_desc *);
>  typedef int (splice_direct_actor)(struct pipe_inode_info *,
>                                 struct splice_desc *);
> +typedef ssize_t (splice_write_actor)(struct pipe_inode_info *,
> +                               struct splice_desc *);
>  
>  extern ssize_t splice_from_pipe(struct pipe_inode_info *, struct file *,
>                               loff_t *, size_t, unsigned int,
> @@ -82,6 +84,9 @@ extern ssize_t splice_to_pipe(struct pipe_inode_info *,
>  extern ssize_t splice_direct_to_actor(struct file *, struct splice_desc *,
>                                     splice_direct_actor *);
>  
> +extern ssize_t splice_write_to_file(struct pipe_inode_info *, struct file *,
> +                                 loff_t *, size_t, unsigned int,
> +                                 splice_write_actor *);
>  /*
>   * for dynamic pipe sizing
>   */
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR

<Prev in Thread] Current Thread [Next in Thread>