* [PATCH TRIVIVAL] ceph: Move the place for EOLDSNAPC handle in ceph_aio_write to easily understand @ 2013-08-08 7:32 majianpeng 2013-08-08 16:40 ` Sage Weil 0 siblings, 1 reply; 4+ messages in thread From: majianpeng @ 2013-08-08 7:32 UTC (permalink / raw) To: sage; +Cc: ceph-devel Only for ceph_sync_write, the osd can return EOLDSNAPC.so move the related codes after the call ceph_sync_write. Signed-off-by: Jianpeng Ma <majianpeng@gmail.com> --- fs/ceph/file.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 2ddf061..30e959f 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -768,6 +768,15 @@ retry_snap: mutex_unlock(&inode->i_mutex); written = ceph_sync_write(file, iov->iov_base, count, pos, &iocb->ki_pos); + if (written == -EOLDSNAPC) { + dout("aio_write %p %llx.%llx %llu~%u" + "got EOLDSNAPC, retrying\n", + inode, ceph_vinop(inode), + pos, (unsigned)iov->iov_len); + mutex_lock(&inode->i_mutex); + hold_mutex = true; + goto retry_snap; + } } else { written = generic_file_buffered_write(iocb, iov, nr_segs, pos, &iocb->ki_pos, @@ -797,14 +806,7 @@ retry_snap: if (err < 0) written = err; } - - if (written == -EOLDSNAPC) { - dout("aio_write %p %llx.%llx %llu~%u got EOLDSNAPC, retrying\n", - inode, ceph_vinop(inode), pos, (unsigned)iov->iov_len); - mutex_lock(&inode->i_mutex); - hold_mutex = true; - goto retry_snap; - } + out: if (hold_mutex) mutex_unlock(&inode->i_mutex); -- 1.8.3.rc1.44.gb387c77 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH TRIVIVAL] ceph: Move the place for EOLDSNAPC handle in ceph_aio_write to easily understand 2013-08-08 7:32 [PATCH TRIVIVAL] ceph: Move the place for EOLDSNAPC handle in ceph_aio_write to easily understand majianpeng @ 2013-08-08 16:40 ` Sage Weil 2013-08-09 2:25 ` majianpeng 0 siblings, 1 reply; 4+ messages in thread From: Sage Weil @ 2013-08-08 16:40 UTC (permalink / raw) To: majianpeng; +Cc: ceph-devel Looks good; I've applied this to the tree. Canyou review the below patch while we are looking at this code? Thanks! sage From 26d0d7b213d87db0ef46e885ae749c27395c11b1 Mon Sep 17 00:00:00 2001 From: Sage Weil <sage@inktank.com> Date: Thu, 8 Aug 2013 09:39:44 -0700 Subject: [PATCH] ceph: replace hold_mutex flag with goto All of the early exit paths need to drop the mutex; it is only the normal path through the function that does not. Skip the unlock in that case with a goto out_unlocked. Signed-off-by: Sage Weil <sage@inktank.com> --- fs/ceph/file.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 7478d5d..a17ffe4 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -710,13 +710,11 @@ static ssize_t ceph_aio_write(struct kiocb *iocb, const struct iovec *iov, &ceph_sb_to_client(inode->i_sb)->client->osdc; ssize_t count, written = 0; int err, want, got; - bool hold_mutex; if (ceph_snap(inode) != CEPH_NOSNAP) return -EROFS; mutex_lock(&inode->i_mutex); - hold_mutex = true; err = generic_segment_checks(iov, &nr_segs, &count, VERIFY_READ); if (err) @@ -772,7 +770,6 @@ retry_snap: inode, ceph_vinop(inode), pos, (unsigned)iov->iov_len); mutex_lock(&inode->i_mutex); - hold_mutex = true; goto retry_snap; } } else { @@ -781,7 +778,6 @@ retry_snap: count, 0); mutex_unlock(&inode->i_mutex); } - hold_mutex = false; if (written >= 0) { int dirty; @@ -805,11 +801,12 @@ retry_snap: written = err; } + goto out_unlocked; + out: - if (hold_mutex) - mutex_unlock(&inode->i_mutex); + mutex_unlock(&inode->i_mutex); +out_unlocked: current->backing_dev_info = NULL; - return written ? written : err; } -- 1.8.1.2 On Thu, 8 Aug 2013, majianpeng wrote: > Only for ceph_sync_write, the osd can return EOLDSNAPC.so move the > related codes after the call ceph_sync_write. > > Signed-off-by: Jianpeng Ma <majianpeng@gmail.com> > --- > fs/ceph/file.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index 2ddf061..30e959f 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -768,6 +768,15 @@ retry_snap: > mutex_unlock(&inode->i_mutex); > written = ceph_sync_write(file, iov->iov_base, count, > pos, &iocb->ki_pos); > + if (written == -EOLDSNAPC) { > + dout("aio_write %p %llx.%llx %llu~%u" > + "got EOLDSNAPC, retrying\n", > + inode, ceph_vinop(inode), > + pos, (unsigned)iov->iov_len); > + mutex_lock(&inode->i_mutex); > + hold_mutex = true; > + goto retry_snap; > + } > } else { > written = generic_file_buffered_write(iocb, iov, nr_segs, > pos, &iocb->ki_pos, > @@ -797,14 +806,7 @@ retry_snap: > if (err < 0) > written = err; > } > - > - if (written == -EOLDSNAPC) { > - dout("aio_write %p %llx.%llx %llu~%u got EOLDSNAPC, retrying\n", > - inode, ceph_vinop(inode), pos, (unsigned)iov->iov_len); > - mutex_lock(&inode->i_mutex); > - hold_mutex = true; > - goto retry_snap; > - } > + > out: > if (hold_mutex) > mutex_unlock(&inode->i_mutex); > -- > 1.8.3.rc1.44.gb387c77 > ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Re: [PATCH TRIVIVAL] ceph: Move the place for EOLDSNAPC handle in ceph_aio_write to easily understand 2013-08-08 16:40 ` Sage Weil @ 2013-08-09 2:25 ` majianpeng 2013-08-09 4:08 ` Sage Weil 0 siblings, 1 reply; 4+ messages in thread From: majianpeng @ 2013-08-09 2:25 UTC (permalink / raw) To: sage; +Cc: ceph-devel >Looks good; I've applied this to the tree. Canyou review the below patch >while we are looking at this code? > >Thanks! >sage > >From 26d0d7b213d87db0ef46e885ae749c27395c11b1 Mon Sep 17 00:00:00 2001 >From: Sage Weil <sage@inktank.com> >Date: Thu, 8 Aug 2013 09:39:44 -0700 >Subject: [PATCH] ceph: replace hold_mutex flag with goto > >All of the early exit paths need to drop the mutex; it is only the normal >path through the function that does not. Skip the unlock in that case >with a goto out_unlocked. > >Signed-off-by: Sage Weil <sage@inktank.com> >--- > fs/ceph/file.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > >diff --git a/fs/ceph/file.c b/fs/ceph/file.c >index 7478d5d..a17ffe4 100644 >--- a/fs/ceph/file.c >+++ b/fs/ceph/file.c >@@ -710,13 +710,11 @@ static ssize_t ceph_aio_write(struct kiocb *iocb, const struct iovec *iov, > &ceph_sb_to_client(inode->i_sb)->client->osdc; > ssize_t count, written = 0; > int err, want, got; >- bool hold_mutex; > > if (ceph_snap(inode) != CEPH_NOSNAP) > return -EROFS; > > mutex_lock(&inode->i_mutex); >- hold_mutex = true; > > err = generic_segment_checks(iov, &nr_segs, &count, VERIFY_READ); > if (err) >@@ -772,7 +770,6 @@ retry_snap: > inode, ceph_vinop(inode), > pos, (unsigned)iov->iov_len); > mutex_lock(&inode->i_mutex); >- hold_mutex = true; > goto retry_snap; > } > } else { >@@ -781,7 +778,6 @@ retry_snap: > count, 0); > mutex_unlock(&inode->i_mutex); > } >- hold_mutex = false; > > if (written >= 0) { > int dirty; >@@ -805,11 +801,12 @@ retry_snap: > written = err; > } > >+ goto out_unlocked; >+ > out: >- if (hold_mutex) >- mutex_unlock(&inode->i_mutex); >+ mutex_unlock(&inode->i_mutex); >+out_unlocked: > current->backing_dev_info = NULL; >- > return written ? written : err; > } > >-- >1.8.1.2 > Hi sage, It's ok. BTW, i had a question about EOLDSNAPC.Now for a sync-write,if it met EOLDSNAPC, it will retry. The rewrite is for all bio.Supported a sync-write crossed multi stripe. Q1:Is there a chance that for some stripe-write it din't met EOLDSNAPC,the later met? Q2:If Q1 occured, can we only rewrite the stripe which met EOLDSNAPCE? Thanks! Jianpeng Ma ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: [PATCH TRIVIVAL] ceph: Move the place for EOLDSNAPC handle in ceph_aio_write to easily understand 2013-08-09 2:25 ` majianpeng @ 2013-08-09 4:08 ` Sage Weil 0 siblings, 0 replies; 4+ messages in thread From: Sage Weil @ 2013-08-09 4:08 UTC (permalink / raw) To: majianpeng; +Cc: ceph-devel On Fri, 9 Aug 2013, majianpeng wrote: > >Looks good; I've applied this to the tree. Canyou review the below patch > >while we are looking at this code? > > > >Thanks! > >sage > > > >From 26d0d7b213d87db0ef46e885ae749c27395c11b1 Mon Sep 17 00:00:00 2001 > >From: Sage Weil <sage@inktank.com> > >Date: Thu, 8 Aug 2013 09:39:44 -0700 > >Subject: [PATCH] ceph: replace hold_mutex flag with goto > > > >All of the early exit paths need to drop the mutex; it is only the normal > >path through the function that does not. Skip the unlock in that case > >with a goto out_unlocked. > > > >Signed-off-by: Sage Weil <sage@inktank.com> > >--- > > fs/ceph/file.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > >diff --git a/fs/ceph/file.c b/fs/ceph/file.c > >index 7478d5d..a17ffe4 100644 > >--- a/fs/ceph/file.c > >+++ b/fs/ceph/file.c > >@@ -710,13 +710,11 @@ static ssize_t ceph_aio_write(struct kiocb *iocb, const struct iovec *iov, > > &ceph_sb_to_client(inode->i_sb)->client->osdc; > > ssize_t count, written = 0; > > int err, want, got; > >- bool hold_mutex; > > > > if (ceph_snap(inode) != CEPH_NOSNAP) > > return -EROFS; > > > > mutex_lock(&inode->i_mutex); > >- hold_mutex = true; > > > > err = generic_segment_checks(iov, &nr_segs, &count, VERIFY_READ); > > if (err) > >@@ -772,7 +770,6 @@ retry_snap: > > inode, ceph_vinop(inode), > > pos, (unsigned)iov->iov_len); > > mutex_lock(&inode->i_mutex); > >- hold_mutex = true; > > goto retry_snap; > > } > > } else { > >@@ -781,7 +778,6 @@ retry_snap: > > count, 0); > > mutex_unlock(&inode->i_mutex); > > } > >- hold_mutex = false; > > > > if (written >= 0) { > > int dirty; > >@@ -805,11 +801,12 @@ retry_snap: > > written = err; > > } > > > >+ goto out_unlocked; > >+ > > out: > >- if (hold_mutex) > >- mutex_unlock(&inode->i_mutex); > >+ mutex_unlock(&inode->i_mutex); > >+out_unlocked: > > current->backing_dev_info = NULL; > >- > > return written ? written : err; > > } > > > >-- > >1.8.1.2 > > > Hi sage, > It's ok. Thanks for reviewing! > BTW, i had a question about EOLDSNAPC.Now for a sync-write,if it met EOLDSNAPC, it will retry. > The rewrite is for all bio.Supported a sync-write crossed multi stripe. > Q1:Is there a chance that for some stripe-write it din't met EOLDSNAPC,the later met? Yes. The writes that cross object boundaries aren't atomic in that sense. > Q2:If Q1 occured, can we only rewrite the stripe which met EOLDSNAPCE? Right. This would mean the write might be 'torn', with half of it in the snapshot and half after. It's not strictly posix, and hard to make atomic without adding a lot of complexity and slowing things down. So far we've decided it's not worth it. sage ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-08-09 4:08 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-08 7:32 [PATCH TRIVIVAL] ceph: Move the place for EOLDSNAPC handle in ceph_aio_write to easily understand majianpeng 2013-08-08 16:40 ` Sage Weil 2013-08-09 2:25 ` majianpeng 2013-08-09 4:08 ` Sage Weil
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.